| Message ID | 20181016220133.26991-8-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | More watermarks improvements | expand |
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 <paulo.r.zanoni@intel.com> > --- > 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(cstate)); > > + 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. Patch looks good to me. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > + 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_state(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_pstate, > + intel_pstate, ddb, > &wm_params, 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 > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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 <paulo.r.zanoni@intel.com> > > --- > > 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. > > Patch looks good to me. > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > + 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 > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
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 <paulo.r.zanoni@intel.com> > > > --- > > > 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ä <ville.syrjala@linux.intel.com> > > > > > + 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 > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > >
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(cstate)); + wp->ddb_blocks = plane_id ? + 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_state(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_pstate, + intel_pstate, ddb, &wm_params, 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)
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 <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_pm.c | 44 ++++++++++++++++------------------------- 2 files changed, 18 insertions(+), 27 deletions(-)