On Fri, Sep 21, 2018 at 07:39:41PM +0200, Maarten Lankhorst wrote:
> Skylake style watermarks program the UV parameters into wm->uv_wm,
> and have a separate DDB allocation for UV blocks into the same plane.
>
> Gen11 watermarks have a separate plane for Y and UV, with separate
> mechanisms. The simplest way to make it work is to keep the current
> way of programming watermarks and calculate the Y and UV plane
> watermarks from the master plane.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
General comment up front: although we're able to re-use a lot of our
gen9/gen10 functions here with relatively minimal code deltas, I feel
like it's costing us a bit in terms of clarity. We're doing a lot of
switching up of plane_id indices based on whether we're dealing with a
non-planar format, a gen9-style planar format, a gen11-style Y plane, or
a gen11-style UV plane, and if feels unusually complicated to keep track
of whether the unified code really is doing the right thing with the
right indices for each of those cases. I think a lot of the complexity
(at least when I run through the logic in my mind) arises because the
hardware design sort of flipped which part of the Y/UV pair is the
"main" content vs the "elsewhere" content from gen9 to gen11 (gen9
treated Y as main and UV as aux, whereas gen11 treats UV as main and Y
as slave).
I'm wondering whether this might be a case where it's simpler and more
maintainable to accept more duplication of code if it allows a more
intuitive split between the gen9-style handling and gen11-style
handling?
> ---
> drivers/gpu/drm/i915/intel_pm.c | 122 +++++++++++++++++++-------------
> 1 file changed, 72 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d76d93452137..b85403798593 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3945,14 +3945,9 @@ skl_ddb_get_hw_plane_state(struct drm_i915_private *dev_priv,
> val & PLANE_CTL_ALPHA_MASK);
>
> val = I915_READ(PLANE_BUF_CFG(pipe, plane_id));
> - /*
> - * FIXME: add proper NV12 support for ICL. Avoid reading unclaimed
> - * registers for now.
> - */
> - if (INTEL_GEN(dev_priv) < 11)
> + if (fourcc == DRM_FORMAT_NV12 && INTEL_GEN(dev_priv) < 11) {
> val2 = I915_READ(PLANE_NV12_BUF_CFG(pipe, plane_id));
>
> - if (fourcc == DRM_FORMAT_NV12) {
> skl_ddb_entry_init_from_hw(dev_priv,
> &ddb->plane[pipe][plane_id], val2);
> skl_ddb_entry_init_from_hw(dev_priv,
> @@ -4207,19 +4202,29 @@ skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate,
> drm_atomic_crtc_state_for_each_plane_state(plane, pstate, cstate) {
> enum plane_id plane_id = to_intel_plane(plane)->id;
> unsigned int rate;
> + struct intel_plane_state *plane_state =
> + to_intel_plane_state(pstate);
> + enum plane_id plane0_id =
> + plane_state->linked_plane ?
> + plane_state->linked_plane->id : plane_id;
> + unsigned int *uv_data_rate =
> + plane_state->linked_plane ?
> + &plane_data_rate[plane_id] :
> + &uv_plane_data_rate[plane_id];
> +
> + if (plane_state->slave)
> + continue;
>
> /* packed/y */
> rate = skl_plane_relative_data_rate(intel_cstate,
> pstate, 0);
> - plane_data_rate[plane_id] = rate;
> -
> + plane_data_rate[plane0_id] = rate;
> total_data_rate += rate;
>
> /* uv-plane */
> rate = skl_plane_relative_data_rate(intel_cstate,
> pstate, 1);
> - uv_plane_data_rate[plane_id] = rate;
> -
> + *uv_data_rate = rate;
> total_data_rate += rate;
> }
>
Like I mentioned above, the changes in this hunk are correct, but
they're deceptively simple for how many different cases we're actually
trying to handle here. If we keep the fully-unified gen9/gen11 code, it
might be worth adding a quick comment before the loop to clarify that we
really have covered all bases (and prevent future code changes from
overlooking and breaking one of these):
/*
* We need to handle four cases here:
* (1) Non-planar pixel format, any platform:
* Data rate stored in plane_data_rate[current plane ID]
* uv_plane_data_rate is filled with 0
*
* (2) NV12 on gen9/gen10 (current plane handles both Y and UV):
* Y data rate stored in plane_data_rate[current plane ID]
* UV data rate stored in uv_plane_data_rate[current plane ID]
*
* (3) NV12 on gen11+ (Y plane content; slave to the UV plane):
* Do nothing since we'll calculate both data rates when
* handling master plane
*
* (4) NV12 on gen11+ (UV plane content; master to the Y plane):
* Y data rate stored in plane_data_rate[slave plane ID]
* UV data rate stored in plane_data_rate[current plane ID]
* uv_plane_data_rate remains unchanged as 0 for both
*/
Similarly, it might be worth adding an assertion to
skl_allocate_pipe_ddb() at the point where we start dealing with
gen9-style uv_plane_blocks, just in case we ever screw this up:
...
uv_data_rate = uv_plane_data_rate[plane_id];
WARN_ON(INTEL_GEN(dev_priv) >= 11 && uv_data_rate > 0);
...
> @@ -4298,6 +4303,7 @@ skl_ddb_calc_min(const struct intel_crtc_state *cstate, int num_active,
>
> drm_atomic_crtc_state_for_each_plane_state(plane, pstate, &cstate->base) {
> enum plane_id plane_id = to_intel_plane(plane)->id;
> + struct intel_plane_state *plane_state = to_intel_plane_state(pstate);
>
> if (plane_id == PLANE_CURSOR)
> continue;
> @@ -4305,8 +4311,14 @@ skl_ddb_calc_min(const struct intel_crtc_state *cstate, int num_active,
> if (!pstate->visible)
> continue;
>
> - minimum[plane_id] = skl_ddb_min_alloc(pstate, 0);
> - uv_minimum[plane_id] = skl_ddb_min_alloc(pstate, 1);
> + if (!plane_state->linked_plane) {
> + minimum[plane_id] = skl_ddb_min_alloc(pstate, 0);
> + uv_minimum[plane_id] = skl_ddb_min_alloc(pstate, 1);
> + } else {
> + enum plane_id aux = plane_state->linked_plane->id;
"aux" might not be the best term here since I believe even on gen11 we
still have aux surfaces, which are something different (e.g., for
CCS_E). Maybe just call this "slave_id" or something?
Matt
> + minimum[aux] = skl_ddb_min_alloc(pstate, 0);
> + minimum[plane_id] = skl_ddb_min_alloc(pstate, 1);
> + }
> }
>
> minimum[PLANE_CURSOR] = skl_cursor_allocation(num_active);
> @@ -4780,36 +4792,20 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
> struct skl_ddb_allocation *ddb,
> struct intel_crtc_state *cstate,
> const struct intel_plane_state *intel_pstate,
> + uint16_t ddb_blocks,
> const struct skl_wm_params *wm_params,
> struct skl_plane_wm *wm,
> - int plane_id)
> + struct skl_wm_level *levels)
> {
> - struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
> - struct drm_plane *plane = intel_pstate->base.plane;
> - struct intel_plane *intel_plane = to_intel_plane(plane);
> - uint16_t ddb_blocks;
> - enum pipe pipe = intel_crtc->pipe;
> int level, max_level = ilk_wm_max_level(dev_priv);
> - enum plane_id intel_plane_id = intel_plane->id;
> + struct skl_wm_level *result_prev = &levels[0];
> int ret;
>
> if (WARN_ON(!intel_pstate->base.fb))
> return -EINVAL;
>
> - ddb_blocks = plane_id ?
> - skl_ddb_entry_size(&ddb->uv_plane[pipe][intel_plane_id]) :
> - skl_ddb_entry_size(&ddb->plane[pipe][intel_plane_id]);
> -
> for (level = 0; level <= max_level; level++) {
> - struct skl_wm_level *result = plane_id ? &wm->uv_wm[level] :
> - &wm->wm[level];
> - struct skl_wm_level *result_prev;
> -
> - if (level)
> - result_prev = plane_id ? &wm->uv_wm[level - 1] :
> - &wm->wm[level - 1];
> - else
> - result_prev = plane_id ? &wm->uv_wm[0] : &wm->wm[0];
> + struct skl_wm_level *result = &levels[level];
>
> ret = skl_compute_plane_wm(dev_priv,
> cstate,
> @@ -4821,6 +4817,8 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
> result);
> if (ret)
> return ret;
> +
> + result_prev = result;
> }
>
> if (intel_pstate->base.fb->format->format == DRM_FORMAT_NV12)
> @@ -4929,20 +4927,29 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
> const struct intel_plane_state *intel_pstate =
> to_intel_plane_state(pstate);
> enum plane_id plane_id = to_intel_plane(plane)->id;
> + enum plane_id plane0_id =
> + intel_pstate->linked_plane ?
> + intel_pstate->linked_plane->id : plane_id;
> struct skl_wm_params wm_params;
> enum pipe pipe = to_intel_crtc(cstate->base.crtc)->pipe;
> uint16_t ddb_blocks;
>
> - wm = &pipe_wm->planes[plane_id];
> - ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][plane_id]);
> + /* Watermarks calculated in master */
> + if (intel_pstate->slave)
> + continue;
> +
> + wm = &pipe_wm->planes[plane0_id];
> + ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][plane0_id]);
>
> ret = skl_compute_plane_wm_params(dev_priv, cstate,
> - intel_pstate, &wm_params, 0);
> + intel_pstate,
> + &wm_params, 0);
> if (ret)
> return ret;
>
> ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
> - intel_pstate, &wm_params, wm, 0);
> + intel_pstate, ddb_blocks,
> + &wm_params, wm, wm->wm);
> if (ret)
> return ret;
>
> @@ -4960,11 +4967,32 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
> if (ret)
> return ret;
>
> - ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
> - intel_pstate, &wm_params,
> - wm, 1);
> - if (ret)
> - return ret;
> + if (intel_pstate->linked_plane) {
> + ddb_blocks =
> + skl_ddb_entry_size(&ddb->plane[pipe][plane_id]);
> +
> + /* write out wm on the UV plane */
> + wm = &pipe_wm->planes[plane_id];
> + wm->is_planar = true;
> +
> + ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
> + intel_pstate, ddb_blocks,
> + &wm_params, wm, wm->wm);
> + if (ret)
> + return ret;
> +
> + skl_compute_transition_wm(cstate, &wm_params, &wm->wm[0],
> + ddb_blocks, &wm->trans_wm);
> + } else {
> + ddb_blocks =
> + skl_ddb_entry_size(&ddb->uv_plane[pipe][plane_id]);
> +
> + ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
> + intel_pstate, ddb_blocks,
> + &wm_params, wm, wm->uv_wm);
> + if (ret)
> + return ret;
> + }
> }
> }
>
> @@ -5016,14 +5044,7 @@ static void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane_id),
> &wm->trans_wm);
>
> - skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane_id),
> - &ddb->plane[pipe][plane_id]);
> - /* FIXME: add proper NV12 support for ICL. */
> - if (INTEL_GEN(dev_priv) >= 11)
> - return skl_ddb_entry_write(dev_priv,
> - PLANE_BUF_CFG(pipe, plane_id),
> - &ddb->plane[pipe][plane_id]);
> - if (wm->is_planar) {
> + if (wm->is_planar && INTEL_GEN(dev_priv) < 11) {
> skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane_id),
> &ddb->uv_plane[pipe][plane_id]);
> skl_ddb_entry_write(dev_priv,
> @@ -5032,7 +5053,8 @@ static void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> } else {
> skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane_id),
> &ddb->plane[pipe][plane_id]);
> - I915_WRITE(PLANE_NV12_BUF_CFG(pipe, plane_id), 0x0);
> + if (INTEL_GEN(dev_priv) < 11)
> + I915_WRITE(PLANE_NV12_BUF_CFG(pipe, plane_id), 0x0);
> }
> }
>
> --
> 2.18.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
