| Message ID | 20220111160812.21344-1-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Recalculate CDCLK if plane scaling ratio changes | expand |
On Tue, 11 Jan 2022, Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> wrote: > Currently we only recalculate CDCLK if active plane mask changes > or if we do a full modeset, however according to BSpec > required Dbuf bandwidth calculations also depend on pipe/plane > scaling ratio, which means that CDCLK must be recalculated > everytime plane scaling ratio changes, because it affects > display buffer bandwidth requirements. > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 63 +++++++++++++++++++- > 1 file changed, 60 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index bf7ce684dd8e..2c616348e993 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -7499,13 +7499,65 @@ static int intel_bigjoiner_add_affected_planes(struct intel_atomic_state *state) > return 0; > } > > +static bool scaling_affects_cdclk(struct intel_plane_state *old_plane_state, > + struct intel_plane_state *new_plane_state) > +{ > + int old_src_w = drm_rect_width(&old_plane_state->uapi.src) >> 16; > + int old_src_h = drm_rect_height(&old_plane_state->uapi.src) >> 16; > + int old_dst_w = drm_rect_width(&old_plane_state->uapi.dst); > + int old_dst_h = drm_rect_height(&old_plane_state->uapi.dst); > + int new_src_w = drm_rect_width(&new_plane_state->uapi.src) >> 16; > + int new_src_h = drm_rect_height(&new_plane_state->uapi.src) >> 16; > + int new_dst_w = drm_rect_width(&new_plane_state->uapi.dst); > + int new_dst_h = drm_rect_height(&new_plane_state->uapi.dst); > + int old_hscale_ratio, new_hscale_ratio; > + int old_vscale_ratio, new_vscale_ratio; > + > + if (needs_scaling(old_plane_state) != needs_scaling(new_plane_state)) > + return true; > + > + if (!old_dst_w || !old_dst_h) > + return true; > + > + DRM_DEBUG_KMS("old_dst_w %d old_dst_h %d\n", old_dst_w, old_dst_h); > + > + old_hscale_ratio = DIV_ROUND_UP(old_src_w, old_dst_w); > + old_vscale_ratio = DIV_ROUND_UP(old_src_h, old_dst_h); > + > + if (!new_dst_w || !new_dst_h) > + return true; > + > + DRM_DEBUG_KMS("new_dst_w %d new_dst_h %d\n", new_dst_w, new_dst_h); > + > + new_hscale_ratio = DIV_ROUND_UP(new_src_w, new_dst_w); > + new_vscale_ratio = DIV_ROUND_UP(new_src_h, new_dst_h); > + > + DRM_DEBUG_KMS("new_hscale_ratio %d new_vscale_ratio %d " > + "old_hscale_ratio %d old_vscale_ratio %d\n", > + new_hscale_ratio, new_vscale_ratio, > + old_hscale_ratio, old_vscale_ratio); All of the debug logging seem excessive? Also, please use drm_dbg_atomic or drm_dbg_kms instead of DRM_DEBUG_KMS for the ones that need to stay. BR, Jani. > + > + if ((old_hscale_ratio != new_hscale_ratio) || > + (old_vscale_ratio != new_vscale_ratio)) { > + DRM_DEBUG_KMS("Scaling ratios changed from %dx%d" > + " to %dx%d - need cdclk recalc\n", > + old_hscale_ratio, old_vscale_ratio, > + new_hscale_ratio, new_vscale_ratio); > + return true; > + } > + > + return false; > +} > + > static int intel_atomic_check_planes(struct intel_atomic_state *state) > { > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > struct intel_crtc_state *old_crtc_state, *new_crtc_state; > struct intel_plane_state *plane_state; > + struct intel_plane_state *old_plane_state; > struct intel_plane *plane; > struct intel_crtc *crtc; > + bool need_cdclk_calc = false; > int i, ret; > > ret = icl_add_linked_planes(state); > @@ -7516,7 +7568,7 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state) > if (ret) > return ret; > > - for_each_new_intel_plane_in_state(state, plane, plane_state, i) { > + for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state, plane_state, i) { > ret = intel_plane_atomic_check(state, plane); > if (ret) { > drm_dbg_atomic(&dev_priv->drm, > @@ -7524,6 +7576,9 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state) > plane->base.base.id, plane->base.name); > return ret; > } > + > + if (scaling_affects_cdclk(old_plane_state, plane_state)) > + need_cdclk_calc = true; > } > > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > @@ -7539,18 +7594,20 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state) > * the planes' minimum cdclk calculation. Add such planes > * to the state before we compute the minimum cdclk. > */ > - if (!active_planes_affects_min_cdclk(dev_priv)) > + if (!active_planes_affects_min_cdclk(dev_priv) && !need_cdclk_calc) > continue; > > old_active_planes = old_crtc_state->active_planes & ~BIT(PLANE_CURSOR); > new_active_planes = new_crtc_state->active_planes & ~BIT(PLANE_CURSOR); > > - if (hweight8(old_active_planes) == hweight8(new_active_planes)) > + if ((hweight8(old_active_planes) == hweight8(new_active_planes)) && !need_cdclk_calc) > continue; > > ret = intel_crtc_add_planes_to_state(state, crtc, new_active_planes); > if (ret) > return ret; > + > + > } > > return 0;
On Tue, Jan 11, 2022 at 06:45:31PM +0200, Jani Nikula wrote: > On Tue, 11 Jan 2022, Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> wrote: > > Currently we only recalculate CDCLK if active plane mask changes > > or if we do a full modeset, however according to BSpec > > required Dbuf bandwidth calculations also depend on pipe/plane > > scaling ratio, which means that CDCLK must be recalculated > > everytime plane scaling ratio changes, because it affects > > display buffer bandwidth requirements. > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 63 +++++++++++++++++++- > > 1 file changed, 60 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > index bf7ce684dd8e..2c616348e993 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -7499,13 +7499,65 @@ static int intel_bigjoiner_add_affected_planes(struct intel_atomic_state *state) > > return 0; > > } > > > > +static bool scaling_affects_cdclk(struct intel_plane_state *old_plane_state, > > + struct intel_plane_state *new_plane_state) > > +{ > > + int old_src_w = drm_rect_width(&old_plane_state->uapi.src) >> 16; > > + int old_src_h = drm_rect_height(&old_plane_state->uapi.src) >> 16; > > + int old_dst_w = drm_rect_width(&old_plane_state->uapi.dst); > > + int old_dst_h = drm_rect_height(&old_plane_state->uapi.dst); > > + int new_src_w = drm_rect_width(&new_plane_state->uapi.src) >> 16; > > + int new_src_h = drm_rect_height(&new_plane_state->uapi.src) >> 16; > > + int new_dst_w = drm_rect_width(&new_plane_state->uapi.dst); > > + int new_dst_h = drm_rect_height(&new_plane_state->uapi.dst); > > + int old_hscale_ratio, new_hscale_ratio; > > + int old_vscale_ratio, new_vscale_ratio; > > + > > + if (needs_scaling(old_plane_state) != needs_scaling(new_plane_state)) > > + return true; > > + > > + if (!old_dst_w || !old_dst_h) > > + return true; > > + > > + DRM_DEBUG_KMS("old_dst_w %d old_dst_h %d\n", old_dst_w, old_dst_h); > > + > > + old_hscale_ratio = DIV_ROUND_UP(old_src_w, old_dst_w); > > + old_vscale_ratio = DIV_ROUND_UP(old_src_h, old_dst_h); > > + > > + if (!new_dst_w || !new_dst_h) > > + return true; > > + > > + DRM_DEBUG_KMS("new_dst_w %d new_dst_h %d\n", new_dst_w, new_dst_h); > > + > > + new_hscale_ratio = DIV_ROUND_UP(new_src_w, new_dst_w); > > + new_vscale_ratio = DIV_ROUND_UP(new_src_h, new_dst_h); > > + > > + DRM_DEBUG_KMS("new_hscale_ratio %d new_vscale_ratio %d " > > + "old_hscale_ratio %d old_vscale_ratio %d\n", > > + new_hscale_ratio, new_vscale_ratio, > > + old_hscale_ratio, old_vscale_ratio); > > All of the debug logging seem excessive? Also, please use drm_dbg_atomic > or drm_dbg_kms instead of DRM_DEBUG_KMS for the ones that need to stay. Yes, I should probably leave only message repoting that cdclk has to be recalculated. Otherwise indeed seems excessive - was just using it for debugging. Stan > > BR, > Jani. > > > + > > + if ((old_hscale_ratio != new_hscale_ratio) || > > + (old_vscale_ratio != new_vscale_ratio)) { > > + DRM_DEBUG_KMS("Scaling ratios changed from %dx%d" > > + " to %dx%d - need cdclk recalc\n", > > + old_hscale_ratio, old_vscale_ratio, > > + new_hscale_ratio, new_vscale_ratio); > > + return true; > > + } > > + > > + return false; > > +} > > + > > static int intel_atomic_check_planes(struct intel_atomic_state *state) > > { > > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > struct intel_crtc_state *old_crtc_state, *new_crtc_state; > > struct intel_plane_state *plane_state; > > + struct intel_plane_state *old_plane_state; > > struct intel_plane *plane; > > struct intel_crtc *crtc; > > + bool need_cdclk_calc = false; > > int i, ret; > > > > ret = icl_add_linked_planes(state); > > @@ -7516,7 +7568,7 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state) > > if (ret) > > return ret; > > > > - for_each_new_intel_plane_in_state(state, plane, plane_state, i) { > > + for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state, plane_state, i) { > > ret = intel_plane_atomic_check(state, plane); > > if (ret) { > > drm_dbg_atomic(&dev_priv->drm, > > @@ -7524,6 +7576,9 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state) > > plane->base.base.id, plane->base.name); > > return ret; > > } > > + > > + if (scaling_affects_cdclk(old_plane_state, plane_state)) > > + need_cdclk_calc = true; > > } > > > > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > > @@ -7539,18 +7594,20 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state) > > * the planes' minimum cdclk calculation. Add such planes > > * to the state before we compute the minimum cdclk. > > */ > > - if (!active_planes_affects_min_cdclk(dev_priv)) > > + if (!active_planes_affects_min_cdclk(dev_priv) && !need_cdclk_calc) > > continue; > > > > old_active_planes = old_crtc_state->active_planes & ~BIT(PLANE_CURSOR); > > new_active_planes = new_crtc_state->active_planes & ~BIT(PLANE_CURSOR); > > > > - if (hweight8(old_active_planes) == hweight8(new_active_planes)) > > + if ((hweight8(old_active_planes) == hweight8(new_active_planes)) && !need_cdclk_calc) > > continue; > > > > ret = intel_crtc_add_planes_to_state(state, crtc, new_active_planes); > > if (ret) > > return ret; > > + > > + > > } > > > > return 0; > > -- > Jani Nikula, Intel Open Source Graphics Center
On Tue, Jan 11, 2022 at 06:08:12PM +0200, Stanislav Lisovskiy wrote: > Currently we only recalculate CDCLK if active plane mask changes > or if we do a full modeset, however according to BSpec > required Dbuf bandwidth calculations also depend on pipe/plane > scaling ratio, which means that CDCLK must be recalculated > everytime plane scaling ratio changes, Already handled by the plane min_cdclk stuff. > because it affects > display buffer bandwidth requirements. Yes, the dbuf bw code is borked. I have old patches on the list that started to fix up all the data rate related stuff, but IIRC I didn't finish it because I ran out of time at the time. I think I have a branch that has a bit more but I'll need to check how far along I actually got in fixing it all... > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 63 +++++++++++++++++++- > 1 file changed, 60 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index bf7ce684dd8e..2c616348e993 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -7499,13 +7499,65 @@ static int intel_bigjoiner_add_affected_planes(struct intel_atomic_state *state) > return 0; > } > > +static bool scaling_affects_cdclk(struct intel_plane_state *old_plane_state, > + struct intel_plane_state *new_plane_state) > +{ > + int old_src_w = drm_rect_width(&old_plane_state->uapi.src) >> 16; > + int old_src_h = drm_rect_height(&old_plane_state->uapi.src) >> 16; > + int old_dst_w = drm_rect_width(&old_plane_state->uapi.dst); > + int old_dst_h = drm_rect_height(&old_plane_state->uapi.dst); > + int new_src_w = drm_rect_width(&new_plane_state->uapi.src) >> 16; > + int new_src_h = drm_rect_height(&new_plane_state->uapi.src) >> 16; > + int new_dst_w = drm_rect_width(&new_plane_state->uapi.dst); > + int new_dst_h = drm_rect_height(&new_plane_state->uapi.dst); > + int old_hscale_ratio, new_hscale_ratio; > + int old_vscale_ratio, new_vscale_ratio; > + > + if (needs_scaling(old_plane_state) != needs_scaling(new_plane_state)) > + return true; > + > + if (!old_dst_w || !old_dst_h) > + return true; > + > + DRM_DEBUG_KMS("old_dst_w %d old_dst_h %d\n", old_dst_w, old_dst_h); > + > + old_hscale_ratio = DIV_ROUND_UP(old_src_w, old_dst_w); > + old_vscale_ratio = DIV_ROUND_UP(old_src_h, old_dst_h); > + > + if (!new_dst_w || !new_dst_h) > + return true; > + > + DRM_DEBUG_KMS("new_dst_w %d new_dst_h %d\n", new_dst_w, new_dst_h); > + > + new_hscale_ratio = DIV_ROUND_UP(new_src_w, new_dst_w); > + new_vscale_ratio = DIV_ROUND_UP(new_src_h, new_dst_h); > + > + DRM_DEBUG_KMS("new_hscale_ratio %d new_vscale_ratio %d " > + "old_hscale_ratio %d old_vscale_ratio %d\n", > + new_hscale_ratio, new_vscale_ratio, > + old_hscale_ratio, old_vscale_ratio); > + > + if ((old_hscale_ratio != new_hscale_ratio) || > + (old_vscale_ratio != new_vscale_ratio)) { > + DRM_DEBUG_KMS("Scaling ratios changed from %dx%d" > + " to %dx%d - need cdclk recalc\n", > + old_hscale_ratio, old_vscale_ratio, > + new_hscale_ratio, new_vscale_ratio); > + return true; > + } > + > + return false; > +} > + > static int intel_atomic_check_planes(struct intel_atomic_state *state) > { > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > struct intel_crtc_state *old_crtc_state, *new_crtc_state; > struct intel_plane_state *plane_state; > + struct intel_plane_state *old_plane_state; > struct intel_plane *plane; > struct intel_crtc *crtc; > + bool need_cdclk_calc = false; > int i, ret; > > ret = icl_add_linked_planes(state); > @@ -7516,7 +7568,7 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state) > if (ret) > return ret; > > - for_each_new_intel_plane_in_state(state, plane, plane_state, i) { > + for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state, plane_state, i) { > ret = intel_plane_atomic_check(state, plane); > if (ret) { > drm_dbg_atomic(&dev_priv->drm, > @@ -7524,6 +7576,9 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state) > plane->base.base.id, plane->base.name); > return ret; > } > + > + if (scaling_affects_cdclk(old_plane_state, plane_state)) > + need_cdclk_calc = true; > } > > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > @@ -7539,18 +7594,20 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state) > * the planes' minimum cdclk calculation. Add such planes > * to the state before we compute the minimum cdclk. > */ > - if (!active_planes_affects_min_cdclk(dev_priv)) > + if (!active_planes_affects_min_cdclk(dev_priv) && !need_cdclk_calc) > continue; > > old_active_planes = old_crtc_state->active_planes & ~BIT(PLANE_CURSOR); > new_active_planes = new_crtc_state->active_planes & ~BIT(PLANE_CURSOR); > > - if (hweight8(old_active_planes) == hweight8(new_active_planes)) > + if ((hweight8(old_active_planes) == hweight8(new_active_planes)) && !need_cdclk_calc) > continue; > > ret = intel_crtc_add_planes_to_state(state, crtc, new_active_planes); > if (ret) > return ret; > + > + > } > > return 0; > -- > 2.24.1.485.gad05a3d8e5
On Wed, Jan 12, 2022 at 03:50:05PM +0200, Ville Syrjälä wrote: > On Tue, Jan 11, 2022 at 06:08:12PM +0200, Stanislav Lisovskiy wrote: > > Currently we only recalculate CDCLK if active plane mask changes > > or if we do a full modeset, however according to BSpec > > required Dbuf bandwidth calculations also depend on pipe/plane > > scaling ratio, which means that CDCLK must be recalculated > > everytime plane scaling ratio changes, > > Already handled by the plane min_cdclk stuff. Problem is that plane min_cdclk will only be called for those which are added to the state. In intel_atomic_check_planes we call intel_crtc_add_planes_to_state only if active_planes_affects_min_cdclk is true and active_planes mask got changed. However if we got one of planes scaling ratio changed, we need to recalculate CDCLK once again and make sure we have all the active planes in state for that. Don't we need all active planes in state to calculate it properly? > > > because it affects > > display buffer bandwidth requirements. > > Yes, the dbuf bw code is borked. I have old patches on the list that > started to fix up all the data rate related stuff, but IIRC I didn't > finish it because I ran out of time at the time. I think I have a > branch that has a bit more but I'll need to check how far along I > actually got in fixing it all... Yeah, it kind of tries to take into account that multiple BSpec requirements into account, however as I remember at the moment when this was committed we were still not even sure, we interpret BSpec properly here. By the way we are still missing cumulative bpp W/A here in upstream. Stan > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 63 +++++++++++++++++++- > > 1 file changed, 60 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > index bf7ce684dd8e..2c616348e993 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -7499,13 +7499,65 @@ static int intel_bigjoiner_add_affected_planes(struct intel_atomic_state *state) > > return 0; > > } > > > > +static bool scaling_affects_cdclk(struct intel_plane_state *old_plane_state, > > + struct intel_plane_state *new_plane_state) > > +{ > > + int old_src_w = drm_rect_width(&old_plane_state->uapi.src) >> 16; > > + int old_src_h = drm_rect_height(&old_plane_state->uapi.src) >> 16; > > + int old_dst_w = drm_rect_width(&old_plane_state->uapi.dst); > > + int old_dst_h = drm_rect_height(&old_plane_state->uapi.dst); > > + int new_src_w = drm_rect_width(&new_plane_state->uapi.src) >> 16; > > + int new_src_h = drm_rect_height(&new_plane_state->uapi.src) >> 16; > > + int new_dst_w = drm_rect_width(&new_plane_state->uapi.dst); > > + int new_dst_h = drm_rect_height(&new_plane_state->uapi.dst); > > + int old_hscale_ratio, new_hscale_ratio; > > + int old_vscale_ratio, new_vscale_ratio; > > + > > + if (needs_scaling(old_plane_state) != needs_scaling(new_plane_state)) > > + return true; > > + > > + if (!old_dst_w || !old_dst_h) > > + return true; > > + > > + DRM_DEBUG_KMS("old_dst_w %d old_dst_h %d\n", old_dst_w, old_dst_h); > > + > > + old_hscale_ratio = DIV_ROUND_UP(old_src_w, old_dst_w); > > + old_vscale_ratio = DIV_ROUND_UP(old_src_h, old_dst_h); > > + > > + if (!new_dst_w || !new_dst_h) > > + return true; > > + > > + DRM_DEBUG_KMS("new_dst_w %d new_dst_h %d\n", new_dst_w, new_dst_h); > > + > > + new_hscale_ratio = DIV_ROUND_UP(new_src_w, new_dst_w); > > + new_vscale_ratio = DIV_ROUND_UP(new_src_h, new_dst_h); > > + > > + DRM_DEBUG_KMS("new_hscale_ratio %d new_vscale_ratio %d " > > + "old_hscale_ratio %d old_vscale_ratio %d\n", > > + new_hscale_ratio, new_vscale_ratio, > > + old_hscale_ratio, old_vscale_ratio); > > + > > + if ((old_hscale_ratio != new_hscale_ratio) || > > + (old_vscale_ratio != new_vscale_ratio)) { > > + DRM_DEBUG_KMS("Scaling ratios changed from %dx%d" > > + " to %dx%d - need cdclk recalc\n", > > + old_hscale_ratio, old_vscale_ratio, > > + new_hscale_ratio, new_vscale_ratio); > > + return true; > > + } > > + > > + return false; > > +} > > + > > static int intel_atomic_check_planes(struct intel_atomic_state *state) > > { > > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > struct intel_crtc_state *old_crtc_state, *new_crtc_state; > > struct intel_plane_state *plane_state; > > + struct intel_plane_state *old_plane_state; > > struct intel_plane *plane; > > struct intel_crtc *crtc; > > + bool need_cdclk_calc = false; > > int i, ret; > > > > ret = icl_add_linked_planes(state); > > @@ -7516,7 +7568,7 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state) > > if (ret) > > return ret; > > > > - for_each_new_intel_plane_in_state(state, plane, plane_state, i) { > > + for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state, plane_state, i) { > > ret = intel_plane_atomic_check(state, plane); > > if (ret) { > > drm_dbg_atomic(&dev_priv->drm, > > @@ -7524,6 +7576,9 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state) > > plane->base.base.id, plane->base.name); > > return ret; > > } > > + > > + if (scaling_affects_cdclk(old_plane_state, plane_state)) > > + need_cdclk_calc = true; > > } > > > > for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, > > @@ -7539,18 +7594,20 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state) > > * the planes' minimum cdclk calculation. Add such planes > > * to the state before we compute the minimum cdclk. > > */ > > - if (!active_planes_affects_min_cdclk(dev_priv)) > > + if (!active_planes_affects_min_cdclk(dev_priv) && !need_cdclk_calc) > > continue; > > > > old_active_planes = old_crtc_state->active_planes & ~BIT(PLANE_CURSOR); > > new_active_planes = new_crtc_state->active_planes & ~BIT(PLANE_CURSOR); > > > > - if (hweight8(old_active_planes) == hweight8(new_active_planes)) > > + if ((hweight8(old_active_planes) == hweight8(new_active_planes)) && !need_cdclk_calc) > > continue; > > > > ret = intel_crtc_add_planes_to_state(state, crtc, new_active_planes); > > if (ret) > > return ret; > > + > > + > > } > > > > return 0; > > -- > > 2.24.1.485.gad05a3d8e5 > > -- > Ville Syrjälä > Intel
On Wed, Jan 12, 2022 at 04:39:17PM +0200, Lisovskiy, Stanislav wrote: > On Wed, Jan 12, 2022 at 03:50:05PM +0200, Ville Syrjälä wrote: > > On Tue, Jan 11, 2022 at 06:08:12PM +0200, Stanislav Lisovskiy wrote: > > > Currently we only recalculate CDCLK if active plane mask changes > > > or if we do a full modeset, however according to BSpec > > > required Dbuf bandwidth calculations also depend on pipe/plane > > > scaling ratio, which means that CDCLK must be recalculated > > > everytime plane scaling ratio changes, > > > > Already handled by the plane min_cdclk stuff. > > Problem is that plane min_cdclk will only be called for those > which are added to the state. > In intel_atomic_check_planes we call intel_crtc_add_planes_to_state > only if active_planes_affects_min_cdclk is true and active_planes > mask got changed. > However if we got one of planes scaling ratio changed, we need to > recalculate CDCLK once again and make sure we have all the active > planes in state for that. Don't we need all active planes > in state to calculate it properly? If the plane's scaling ratio is changing then that plane is already in the state. The min_cdclk/data_rate/etc. are all then cached in the crtc state so that plane isn't needed again until its scaling ratio (or whatever else) changes again.
On Wed, Jan 12, 2022 at 04:50:01PM +0200, Ville Syrjälä wrote: > On Wed, Jan 12, 2022 at 04:39:17PM +0200, Lisovskiy, Stanislav wrote: > > On Wed, Jan 12, 2022 at 03:50:05PM +0200, Ville Syrjälä wrote: > > > On Tue, Jan 11, 2022 at 06:08:12PM +0200, Stanislav Lisovskiy wrote: > > > > Currently we only recalculate CDCLK if active plane mask changes > > > > or if we do a full modeset, however according to BSpec > > > > required Dbuf bandwidth calculations also depend on pipe/plane > > > > scaling ratio, which means that CDCLK must be recalculated > > > > everytime plane scaling ratio changes, > > > > > > Already handled by the plane min_cdclk stuff. > > > > Problem is that plane min_cdclk will only be called for those > > which are added to the state. > > In intel_atomic_check_planes we call intel_crtc_add_planes_to_state > > only if active_planes_affects_min_cdclk is true and active_planes > > mask got changed. > > However if we got one of planes scaling ratio changed, we need to > > recalculate CDCLK once again and make sure we have all the active > > planes in state for that. Don't we need all active planes > > in state to calculate it properly? > > If the plane's scaling ratio is changing then that plane is already > in the state. The min_cdclk/data_rate/etc. are all then cached in > the crtc state so that plane isn't needed again until its scaling > ratio (or whatever else) changes again. Yep, was just wondering that according to this logic why we do call intel_crtc_add_planes_to_state, once active plane mask changes then. Stan > > -- > Ville Syrjälä > Intel
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index bf7ce684dd8e..2c616348e993 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -7499,13 +7499,65 @@ static int intel_bigjoiner_add_affected_planes(struct intel_atomic_state *state) return 0; } +static bool scaling_affects_cdclk(struct intel_plane_state *old_plane_state, + struct intel_plane_state *new_plane_state) +{ + int old_src_w = drm_rect_width(&old_plane_state->uapi.src) >> 16; + int old_src_h = drm_rect_height(&old_plane_state->uapi.src) >> 16; + int old_dst_w = drm_rect_width(&old_plane_state->uapi.dst); + int old_dst_h = drm_rect_height(&old_plane_state->uapi.dst); + int new_src_w = drm_rect_width(&new_plane_state->uapi.src) >> 16; + int new_src_h = drm_rect_height(&new_plane_state->uapi.src) >> 16; + int new_dst_w = drm_rect_width(&new_plane_state->uapi.dst); + int new_dst_h = drm_rect_height(&new_plane_state->uapi.dst); + int old_hscale_ratio, new_hscale_ratio; + int old_vscale_ratio, new_vscale_ratio; + + if (needs_scaling(old_plane_state) != needs_scaling(new_plane_state)) + return true; + + if (!old_dst_w || !old_dst_h) + return true; + + DRM_DEBUG_KMS("old_dst_w %d old_dst_h %d\n", old_dst_w, old_dst_h); + + old_hscale_ratio = DIV_ROUND_UP(old_src_w, old_dst_w); + old_vscale_ratio = DIV_ROUND_UP(old_src_h, old_dst_h); + + if (!new_dst_w || !new_dst_h) + return true; + + DRM_DEBUG_KMS("new_dst_w %d new_dst_h %d\n", new_dst_w, new_dst_h); + + new_hscale_ratio = DIV_ROUND_UP(new_src_w, new_dst_w); + new_vscale_ratio = DIV_ROUND_UP(new_src_h, new_dst_h); + + DRM_DEBUG_KMS("new_hscale_ratio %d new_vscale_ratio %d " + "old_hscale_ratio %d old_vscale_ratio %d\n", + new_hscale_ratio, new_vscale_ratio, + old_hscale_ratio, old_vscale_ratio); + + if ((old_hscale_ratio != new_hscale_ratio) || + (old_vscale_ratio != new_vscale_ratio)) { + DRM_DEBUG_KMS("Scaling ratios changed from %dx%d" + " to %dx%d - need cdclk recalc\n", + old_hscale_ratio, old_vscale_ratio, + new_hscale_ratio, new_vscale_ratio); + return true; + } + + return false; +} + static int intel_atomic_check_planes(struct intel_atomic_state *state) { struct drm_i915_private *dev_priv = to_i915(state->base.dev); struct intel_crtc_state *old_crtc_state, *new_crtc_state; struct intel_plane_state *plane_state; + struct intel_plane_state *old_plane_state; struct intel_plane *plane; struct intel_crtc *crtc; + bool need_cdclk_calc = false; int i, ret; ret = icl_add_linked_planes(state); @@ -7516,7 +7568,7 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state) if (ret) return ret; - for_each_new_intel_plane_in_state(state, plane, plane_state, i) { + for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state, plane_state, i) { ret = intel_plane_atomic_check(state, plane); if (ret) { drm_dbg_atomic(&dev_priv->drm, @@ -7524,6 +7576,9 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state) plane->base.base.id, plane->base.name); return ret; } + + if (scaling_affects_cdclk(old_plane_state, plane_state)) + need_cdclk_calc = true; } for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, @@ -7539,18 +7594,20 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state) * the planes' minimum cdclk calculation. Add such planes * to the state before we compute the minimum cdclk. */ - if (!active_planes_affects_min_cdclk(dev_priv)) + if (!active_planes_affects_min_cdclk(dev_priv) && !need_cdclk_calc) continue; old_active_planes = old_crtc_state->active_planes & ~BIT(PLANE_CURSOR); new_active_planes = new_crtc_state->active_planes & ~BIT(PLANE_CURSOR); - if (hweight8(old_active_planes) == hweight8(new_active_planes)) + if ((hweight8(old_active_planes) == hweight8(new_active_planes)) && !need_cdclk_calc) continue; ret = intel_crtc_add_planes_to_state(state, crtc, new_active_planes); if (ret) return ret; + + } return 0;
Currently we only recalculate CDCLK if active plane mask changes or if we do a full modeset, however according to BSpec required Dbuf bandwidth calculations also depend on pipe/plane scaling ratio, which means that CDCLK must be recalculated everytime plane scaling ratio changes, because it affects display buffer bandwidth requirements. Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 63 +++++++++++++++++++- 1 file changed, 60 insertions(+), 3 deletions(-)