On Mon, Oct 22, 2018 at 03:29:22PM -0700, Paulo Zanoni wrote: > Em Qui, 2018-10-18 às 16:41 +0300, Ville Syrjälä escreveu: > > On Tue, Oct 16, 2018 at 03:01:29PM -0700, Paulo Zanoni wrote: > > > The goal of struct skl_wm_params is to cache every watermark > > > parameter so the other functions can just use them without worrying > > > about the appropriate place to fetch each parameter requested by > > > the > > > spec, and without having to recompute parameters that are used in > > > different steps of the calculation. > > > > > > The ddb_blocks parameter is one that is used by both the the plane > > > watermarks and the transition watermarks. Move ddb_blocks to the > > > parameter struct so we can simplify the code. > > > > > > Signed-off-by: Paulo Zanoni <[email protected]> > > > --- > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > drivers/gpu/drm/i915/intel_pm.c | 44 ++++++++++++++++------------- > > > ------------ > > > 2 files changed, 18 insertions(+), 27 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > > b/drivers/gpu/drm/i915/i915_drv.h > > > index 4b1e8471609b..b32d680d9bf0 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > @@ -1255,6 +1255,7 @@ struct skl_wm_params { > > > bool rc_surface; > > > bool is_planar; > > > uint32_t width; > > > + uint16_t ddb_blocks; > > > uint8_t cpp; > > > uint32_t plane_pixel_rate; > > > uint32_t y_min_scanlines; > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > > > b/drivers/gpu/drm/i915/intel_pm.c > > > index f388bfa99a97..4053f4a68657 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -4522,11 +4522,13 @@ static int > > > skl_compute_plane_wm_params(const struct drm_i915_private > > > *dev_priv, > > > struct intel_crtc_state *cstate, > > > const struct intel_plane_state > > > *intel_pstate, > > > + const struct skl_ddb_allocation *ddb, > > > struct skl_wm_params *wp, int > > > plane_id) > > > { > > > struct intel_plane *plane = to_intel_plane(intel_pstate- > > > >base.plane); > > > const struct drm_plane_state *pstate = &intel_pstate- > > > >base; > > > const struct drm_framebuffer *fb = pstate->fb; > > > + enum pipe pipe = to_intel_crtc(cstate->base.crtc)->pipe; > > > uint32_t interm_pbpl; > > > struct intel_atomic_state *state = > > > to_intel_atomic_state(cstate->base.state); > > > @@ -4624,13 +4626,16 @@ skl_compute_plane_wm_params(const struct > > > drm_i915_private *dev_priv, > > > wp->linetime_us = fixed16_to_u32_round_up( > > > intel_get_linetime_us(csta > > > te)); > > > > > > + wp->ddb_blocks = plane_id ? > > > > Ugh. That 'plane_id' really should be renamed to something else. > > 'color_plane' would be the newfangled name I started to use > > elsewhere. > > I completely agree: there's plane_id and plane->id and they mean > different things. > > If you're already using color_plane I can totally go with it so we have > consistency. But, well, for RGB planes, plane 0 does contain color > information... But can I suggest us to use "subplane" terminology when > talking about the planes of a plane? What about subplane_index? > Something that does not use the word "plane" is probably better.
What is "planes of a plane"? There are drm_planes, hardware display planes (1:1 with drm_plane for us), and there are the planes of a planar framebuffer (luma plane, chroma plane, etc.). The latter is what I christened "color plane". > > > > > Patch looks good to me. > > Reviewed-by: Ville Syrjälä <[email protected]> > > > > > + skl_ddb_entry_size(&ddb- > > > >uv_plane[pipe][plane->id]) : > > > + skl_ddb_entry_size(&ddb->plane[pipe][plane- > > > >id]); > > > + > > > return 0; > > > } > > > > > > static int skl_compute_plane_wm(const struct drm_i915_private > > > *dev_priv, > > > struct intel_crtc_state *cstate, > > > const struct intel_plane_state > > > *intel_pstate, > > > - uint16_t ddb_allocation, > > > int level, > > > const struct skl_wm_params *wp, > > > const struct skl_wm_level > > > *result_prev, > > > @@ -4674,7 +4679,7 @@ static int skl_compute_plane_wm(const struct > > > drm_i915_private *dev_priv, > > > wp->dbuf_block_size < 1) && > > > (wp->plane_bytes_per_line / wp- > > > >dbuf_block_size < 1)) { > > > selected_result = method2; > > > - } else if (ddb_allocation >= > > > + } else if (wp->ddb_blocks >= > > > fixed16_to_u32_round_up(wp- > > > >plane_blocks_per_line)) { > > > if (INTEL_GEN(dev_priv) == 9 && > > > !IS_GEMINILAKE(dev_priv)) > > > @@ -4747,8 +4752,8 @@ static int skl_compute_plane_wm(const struct > > > drm_i915_private *dev_priv, > > > } > > > > > > if ((level > 0 && res_lines > 31) || > > > - res_blocks >= ddb_allocation || > > > - min_disp_buf_needed >= ddb_allocation) { > > > + res_blocks >= wp->ddb_blocks || > > > + min_disp_buf_needed >= wp->ddb_blocks) { > > > result->plane_en = false; > > > > > > /* > > > @@ -4763,7 +4768,7 @@ static int skl_compute_plane_wm(const struct > > > drm_i915_private *dev_priv, > > > DRM_DEBUG_KMS("Requested display > > > configuration exceeds system watermark limitations\n"); > > > DRM_DEBUG_KMS("[PLANE:%d:%s] blocks > > > required = %u/%u, lines required = %u/31\n", > > > plane->base.id, plane->name, > > > - res_blocks, ddb_allocation, > > > res_lines); > > > + res_blocks, wp->ddb_blocks, > > > res_lines); > > > return -EINVAL; > > > } > > > } > > > @@ -4789,26 +4794,15 @@ static int skl_compute_plane_wm(const > > > struct drm_i915_private *dev_priv, > > > > > > static int > > > 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, > > > const struct skl_wm_params *wm_params, > > > struct skl_plane_wm *wm, > > > int plane_id) > > > { > > > - 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; > > > int ret; > > > > > > - 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]; > > > @@ -4823,7 +4817,6 @@ skl_compute_wm_levels(const struct > > > drm_i915_private *dev_priv, > > > ret = skl_compute_plane_wm(dev_priv, > > > cstate, > > > intel_pstate, > > > - ddb_blocks, > > > level, > > > wm_params, > > > result_prev, > > > @@ -4863,7 +4856,6 @@ skl_compute_linetime_wm(struct > > > intel_crtc_state *cstate) > > > static void skl_compute_transition_wm(struct intel_crtc_state > > > *cstate, > > > struct skl_wm_params *wp, > > > struct skl_wm_level *wm_l0, > > > - uint16_t ddb_allocation, > > > struct skl_wm_level > > > *trans_wm /* out */) > > > { > > > struct drm_device *dev = cstate->base.crtc->dev; > > > @@ -4914,7 +4906,7 @@ static void skl_compute_transition_wm(struct > > > intel_crtc_state *cstate, > > > > > > res_blocks += 1; > > > > > > - if (res_blocks < ddb_allocation) { > > > + if (res_blocks < wp->ddb_blocks) { > > > trans_wm->plane_res_b = res_blocks; > > > trans_wm->plane_en = true; > > > return; > > > @@ -4947,37 +4939,35 @@ static int skl_build_pipe_wm(struct > > > intel_crtc_state *cstate, > > > to_intel_plane_sta > > > te(pstate); > > > enum plane_id plane_id = to_intel_plane(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]); > > > > > > ret = skl_compute_plane_wm_params(dev_priv, > > > cstate, > > > - intel_pstate, > > > &wm_params, 0); > > > + intel_pstate, > > > ddb, > > > + &wm_params, 0); > > > if (ret) > > > return ret; > > > > > > if (!wm_params.plane_visible) > > > continue; > > > > > > - ret = skl_compute_wm_levels(dev_priv, ddb, cstate, > > > + ret = skl_compute_wm_levels(dev_priv, cstate, > > > intel_pstate, > > > &wm_params, wm, 0); > > > if (ret) > > > return ret; > > > > > > skl_compute_transition_wm(cstate, &wm_params, &wm- > > > >wm[0], > > > - ddb_blocks, &wm- > > > >trans_wm); > > > + &wm->trans_wm); > > > > > > /* uv plane watermarks must also be validated for > > > NV12/Planar */ > > > if (wm_params.is_planar) { > > > ret = > > > skl_compute_plane_wm_params(dev_priv, cstate, > > > - intel_ps > > > tate, > > > + intel_ps > > > tate, ddb, > > > &wm_para > > > ms, 1); > > > if (ret) > > > return ret; > > > > > > - ret = skl_compute_wm_levels(dev_priv, ddb, > > > cstate, > > > + ret = skl_compute_wm_levels(dev_priv, > > > cstate, > > > intel_pstate, > > > &wm_params, > > > wm, 1); > > > if (ret) > > > -- > > > 2.14.4 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > [email protected] > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- Ville Syrjälä Intel _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
