Message ID | 20200309161204.17792-2-stanislav.lisovskiy@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Refactor Gen11+ SAGV support | expand |
On Mon, Mar 09, 2020 at 06:11:57PM +0200, Stanislav Lisovskiy wrote: > We need to start passing memory latency as a > parameter when calculating plane wm levels, > as latency can get changed in different > circumstances(for example with or without SAGV). > So we need to be more flexible on that matter. > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 8375054ba27d..c7928c870b0a 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -4016,6 +4016,7 @@ static int skl_compute_wm_params(const struct intel_crtc_state *crtc_state, > int color_plane); > static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state, > int level, > + u32 latency, So you didn't change the types? > const struct skl_wm_params *wp, > const struct skl_wm_level *result_prev, > struct skl_wm_level *result /* out */); > @@ -4038,7 +4039,9 @@ skl_cursor_allocation(const struct intel_crtc_state *crtc_state, > drm_WARN_ON(&dev_priv->drm, ret); > > for (level = 0; level <= max_level; level++) { > - skl_compute_plane_wm(crtc_state, level, &wp, &wm, &wm); > + u32 latency = dev_priv->wm.skl_latency[level]; > + > + skl_compute_plane_wm(crtc_state, level, latency, &wp, &wm, &wm); > if (wm.min_ddb_alloc == U16_MAX) > break; > > @@ -4972,12 +4975,12 @@ static bool skl_wm_has_lines(struct drm_i915_private *dev_priv, int level) > > static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state, > int level, > + u32 latency, > const struct skl_wm_params *wp, > const struct skl_wm_level *result_prev, > struct skl_wm_level *result /* out */) > { > struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev); > - u32 latency = dev_priv->wm.skl_latency[level]; > uint_fixed_16_16_t method1, method2; > uint_fixed_16_16_t selected_result; > u32 res_blocks, res_lines, min_ddb_alloc = 0; > @@ -5106,9 +5109,10 @@ skl_compute_wm_levels(const struct intel_crtc_state *crtc_state, > > for (level = 0; level <= max_level; level++) { > struct skl_wm_level *result = &levels[level]; > + u32 latency = dev_priv->wm.skl_latency[level]; > > - skl_compute_plane_wm(crtc_state, level, wm_params, > - result_prev, result); > + skl_compute_plane_wm(crtc_state, level, latency, > + wm_params, result_prev, result); > > result_prev = result; > } > -- > 2.24.1.485.gad05a3d8e5
On Tue, 2020-03-10 at 16:32 +0200, Ville Syrjälä wrote: > On Mon, Mar 09, 2020 at 06:11:57PM +0200, Stanislav Lisovskiy wrote: > > We need to start passing memory latency as a > > parameter when calculating plane wm levels, > > as latency can get changed in different > > circumstances(for example with or without SAGV). > > So we need to be more flexible on that matter. > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > --- > > drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++---- > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > > b/drivers/gpu/drm/i915/intel_pm.c > > index 8375054ba27d..c7928c870b0a 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -4016,6 +4016,7 @@ static int skl_compute_wm_params(const struct > > intel_crtc_state *crtc_state, > > int color_plane); > > static void skl_compute_plane_wm(const struct intel_crtc_state > > *crtc_state, > > int level, > > + u32 latency, > > So you didn't change the types? Yes, I saw your comment there - and looked into this, however I just wondered, does it make any sense do to that. The reason is because skl_latency is anyway defined as u16 in i915_drv.h, just as pri/spr/cur latencies, so wonder how this "unsigned int" going to fit into this. Should I maybe then change it to u16 - at least that would somehow comply with the current declarations. > > > const struct skl_wm_params *wp, > > const struct skl_wm_level > > *result_prev, > > struct skl_wm_level *result /* out > > */); > > @@ -4038,7 +4039,9 @@ skl_cursor_allocation(const struct > > intel_crtc_state *crtc_state, > > drm_WARN_ON(&dev_priv->drm, ret); > > > > for (level = 0; level <= max_level; level++) { > > - skl_compute_plane_wm(crtc_state, level, &wp, &wm, &wm); > > + u32 latency = dev_priv->wm.skl_latency[level]; > > + > > + skl_compute_plane_wm(crtc_state, level, latency, &wp, > > &wm, &wm); > > if (wm.min_ddb_alloc == U16_MAX) > > break; > > > > @@ -4972,12 +4975,12 @@ static bool skl_wm_has_lines(struct > > drm_i915_private *dev_priv, int level) > > > > static void skl_compute_plane_wm(const struct intel_crtc_state > > *crtc_state, > > int level, > > + u32 latency, > > const struct skl_wm_params *wp, > > const struct skl_wm_level > > *result_prev, > > struct skl_wm_level *result /* out */) > > { > > struct drm_i915_private *dev_priv = to_i915(crtc_state- > > >uapi.crtc->dev); > > - u32 latency = dev_priv->wm.skl_latency[level]; > > uint_fixed_16_16_t method1, method2; > > uint_fixed_16_16_t selected_result; > > u32 res_blocks, res_lines, min_ddb_alloc = 0; > > @@ -5106,9 +5109,10 @@ skl_compute_wm_levels(const struct > > intel_crtc_state *crtc_state, > > > > for (level = 0; level <= max_level; level++) { > > struct skl_wm_level *result = &levels[level]; > > + u32 latency = dev_priv->wm.skl_latency[level]; > > > > - skl_compute_plane_wm(crtc_state, level, wm_params, > > - result_prev, result); > > + skl_compute_plane_wm(crtc_state, level, latency, > > + wm_params, result_prev, result); > > > > result_prev = result; > > } > > -- > > 2.24.1.485.gad05a3d8e5 > >
On Tue, Mar 10, 2020 at 02:54:12PM +0000, Lisovskiy, Stanislav wrote: > On Tue, 2020-03-10 at 16:32 +0200, Ville Syrjälä wrote: > > On Mon, Mar 09, 2020 at 06:11:57PM +0200, Stanislav Lisovskiy wrote: > > > We need to start passing memory latency as a > > > parameter when calculating plane wm levels, > > > as latency can get changed in different > > > circumstances(for example with or without SAGV). > > > So we need to be more flexible on that matter. > > > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++---- > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > > > b/drivers/gpu/drm/i915/intel_pm.c > > > index 8375054ba27d..c7928c870b0a 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -4016,6 +4016,7 @@ static int skl_compute_wm_params(const struct > > > intel_crtc_state *crtc_state, > > > int color_plane); > > > static void skl_compute_plane_wm(const struct intel_crtc_state > > > *crtc_state, > > > int level, > > > + u32 latency, > > > > So you didn't change the types? > > Yes, I saw your comment there - and looked into this, however I just > wondered, does it make any sense do to that. The reason is because > skl_latency is anyway defined as u16 in i915_drv.h, just as pri/spr/cur > latencies, so wonder how this "unsigned int" going to fit into this. > Should I maybe then change it to u16 - at least that would somehow > comply with the current declarations. It's u16 in the struct to not waste space. In the code it's just a number so a sized type doesn't make all that much sense. And I think most of the code uses int/unsigned int for it anyway.
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 8375054ba27d..c7928c870b0a 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4016,6 +4016,7 @@ static int skl_compute_wm_params(const struct intel_crtc_state *crtc_state, int color_plane); static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state, int level, + u32 latency, const struct skl_wm_params *wp, const struct skl_wm_level *result_prev, struct skl_wm_level *result /* out */); @@ -4038,7 +4039,9 @@ skl_cursor_allocation(const struct intel_crtc_state *crtc_state, drm_WARN_ON(&dev_priv->drm, ret); for (level = 0; level <= max_level; level++) { - skl_compute_plane_wm(crtc_state, level, &wp, &wm, &wm); + u32 latency = dev_priv->wm.skl_latency[level]; + + skl_compute_plane_wm(crtc_state, level, latency, &wp, &wm, &wm); if (wm.min_ddb_alloc == U16_MAX) break; @@ -4972,12 +4975,12 @@ static bool skl_wm_has_lines(struct drm_i915_private *dev_priv, int level) static void skl_compute_plane_wm(const struct intel_crtc_state *crtc_state, int level, + u32 latency, const struct skl_wm_params *wp, const struct skl_wm_level *result_prev, struct skl_wm_level *result /* out */) { struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev); - u32 latency = dev_priv->wm.skl_latency[level]; uint_fixed_16_16_t method1, method2; uint_fixed_16_16_t selected_result; u32 res_blocks, res_lines, min_ddb_alloc = 0; @@ -5106,9 +5109,10 @@ skl_compute_wm_levels(const struct intel_crtc_state *crtc_state, for (level = 0; level <= max_level; level++) { struct skl_wm_level *result = &levels[level]; + u32 latency = dev_priv->wm.skl_latency[level]; - skl_compute_plane_wm(crtc_state, level, wm_params, - result_prev, result); + skl_compute_plane_wm(crtc_state, level, latency, + wm_params, result_prev, result); result_prev = result; }