| Message ID | 20240325112329.7922-2-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable fastset for mbus_join state change | expand |
On Mon, Mar 25, 2024 at 01:23:26PM +0200, Stanislav Lisovskiy wrote: > According to BSpec we need to do correspondent MBUS updates before > or after DBUF reallocation, depending on whether we are enabling > or disabling mbus joining(typical scenario is swithing between > multiple and single displays). > > Also we need to be able to update dbuf min tracker and mdclk ratio > separately if mbus_join state didn't change, so lets add one > degree of freedom and make it possible. > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/i915/display/skl_watermark.c | 54 +++++++++++++------- > 1 file changed, 35 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c > index bc341abcab2fe..2b947870527fc 100644 > --- a/drivers/gpu/drm/i915/display/skl_watermark.c > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c > @@ -3570,16 +3570,38 @@ void intel_dbuf_mdclk_cdclk_ratio_update(struct drm_i915_private *i915, u8 ratio > DBUF_MIN_TRACKER_STATE_SERVICE(ratio - 1)); > } > > +static void intel_dbuf_mdclk_min_tracker_update(struct intel_atomic_state *state) > +{ > + struct drm_i915_private *i915 = to_i915(state->base.dev); > + const struct intel_dbuf_state *old_dbuf_state = > + intel_atomic_get_old_dbuf_state(state); > + const struct intel_dbuf_state *new_dbuf_state = > + intel_atomic_get_new_dbuf_state(state); > + > + if (DISPLAY_VER(i915) >= 20 && > + old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) { > + /* > + * For Xe2LPD and beyond, when there is a change in the ratio > + * between MDCLK and CDCLK, updates to related registers need to > + * happen at a specific point in the CDCLK change sequence. In > + * that case, we defer to the call to > + * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic. > + */ > + return; > + } That still needs to be removed or else we'll not update the ratio at all during the mbus_join changes. I don't think I saw any removal in subsequent patches. > + > + intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio, And it just occurred to me that this thing will in fact be wrong during the pre/post ddb hooks *and* cdclk is getting decreased from the post plane update hook. I can't immediately think of a super nice way to handle this. Perhaps the most stragithforward idea is to just get the mdclk/cdclk ratio from i915->display.cdclk.hw during the pre/post ddb hooks. cdclk serialization should guard against parallel updates from two both places and thus isplay.cdclk.hw should be safe to use. The other option would be to determine if a cdclk decrease is going to happen or not, and depending on that use the old vs. new dbuf_state when updating the ratio in the pre/post ddb hooks. > + new_dbuf_state->joined_mbus); > +} > + > /* > * Configure MBUS_CTL and all DBUF_CTL_S of each slice to join_mbus state before > * update the request state of all DBUS slices. > */ > -static void update_mbus_pre_enable(struct intel_atomic_state *state) > +static void intel_dbuf_mbus_join_update(struct intel_atomic_state *state) > { > struct drm_i915_private *i915 = to_i915(state->base.dev); > u32 mbus_ctl; > - const struct intel_dbuf_state *old_dbuf_state = > - intel_atomic_get_old_dbuf_state(state); > const struct intel_dbuf_state *new_dbuf_state = > intel_atomic_get_new_dbuf_state(state); > > @@ -3600,21 +3622,6 @@ static void update_mbus_pre_enable(struct intel_atomic_state *state) > intel_de_rmw(i915, MBUS_CTL, > MBUS_HASHING_MODE_MASK | MBUS_JOIN | > MBUS_JOIN_PIPE_SELECT_MASK, mbus_ctl); > - > - if (DISPLAY_VER(i915) >= 20 && > - old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) { > - /* > - * For Xe2LPD and beyond, when there is a change in the ratio > - * between MDCLK and CDCLK, updates to related registers need to > - * happen at a specific point in the CDCLK change sequence. In > - * that case, we defer to the call to > - * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic. > - */ > - return; > - } > - > - intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio, > - new_dbuf_state->joined_mbus); > } > > void intel_dbuf_pre_plane_update(struct intel_atomic_state *state) > @@ -3632,7 +3639,11 @@ void intel_dbuf_pre_plane_update(struct intel_atomic_state *state) > > WARN_ON(!new_dbuf_state->base.changed); > > - update_mbus_pre_enable(state); > + if (!old_dbuf_state->joined_mbus && new_dbuf_state->joined_mbus) { I think you squashed that stuff into the wrong patch. This one should have a pure refactoring patch. > + intel_dbuf_mbus_join_update(state); > + intel_dbuf_mdclk_min_tracker_update(state); > + } > + > gen9_dbuf_slices_update(i915, > old_dbuf_state->enabled_slices | > new_dbuf_state->enabled_slices); > @@ -3653,6 +3664,11 @@ void intel_dbuf_post_plane_update(struct intel_atomic_state *state) > > WARN_ON(!new_dbuf_state->base.changed); > > + if (old_dbuf_state->joined_mbus && !new_dbuf_state->joined_mbus) { > + intel_dbuf_mbus_join_update(state); > + intel_dbuf_mdclk_min_tracker_update(state); > + } > + > gen9_dbuf_slices_update(i915, > new_dbuf_state->enabled_slices); > } > -- > 2.37.3
On Mon, Mar 25, 2024 at 04:45:49PM +0200, Ville Syrjälä wrote: > On Mon, Mar 25, 2024 at 01:23:26PM +0200, Stanislav Lisovskiy wrote: > > According to BSpec we need to do correspondent MBUS updates before > > or after DBUF reallocation, depending on whether we are enabling > > or disabling mbus joining(typical scenario is swithing between > > multiple and single displays). > > > > Also we need to be able to update dbuf min tracker and mdclk ratio > > separately if mbus_join state didn't change, so lets add one > > degree of freedom and make it possible. > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > --- > > drivers/gpu/drm/i915/display/skl_watermark.c | 54 +++++++++++++------- > > 1 file changed, 35 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c > > index bc341abcab2fe..2b947870527fc 100644 > > --- a/drivers/gpu/drm/i915/display/skl_watermark.c > > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c > > @@ -3570,16 +3570,38 @@ void intel_dbuf_mdclk_cdclk_ratio_update(struct drm_i915_private *i915, u8 ratio > > DBUF_MIN_TRACKER_STATE_SERVICE(ratio - 1)); > > } > > > > +static void intel_dbuf_mdclk_min_tracker_update(struct intel_atomic_state *state) > > +{ > > + struct drm_i915_private *i915 = to_i915(state->base.dev); > > + const struct intel_dbuf_state *old_dbuf_state = > > + intel_atomic_get_old_dbuf_state(state); > > + const struct intel_dbuf_state *new_dbuf_state = > > + intel_atomic_get_new_dbuf_state(state); > > + > > + if (DISPLAY_VER(i915) >= 20 && > > + old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) { > > + /* > > + * For Xe2LPD and beyond, when there is a change in the ratio > > + * between MDCLK and CDCLK, updates to related registers need to > > + * happen at a specific point in the CDCLK change sequence. In > > + * that case, we defer to the call to > > + * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic. > > + */ > > + return; > > + } > > That still needs to be removed or else we'll not update the ratio at > all during the mbus_join changes. I don't think I saw any removal > in subsequent patches. > > > + > > + intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio, I don't get what is happening here. "That whole condition I think needs to go. We want to update the ratio also when changing mbus joining. But that behavioural change doesn't really belong in this patch, so this is Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>" Now it again needs to be changed or changed in other patch(in this series or which one), I don't follow. Should it be the patch changing MBUS join value? Stan > > And it just occurred to me that this thing will in fact be wrong > during the pre/post ddb hooks *and* cdclk is getting decreased > from the post plane update hook. > > I can't immediately think of a super nice way to handle this. > > Perhaps the most stragithforward idea is to just get the mdclk/cdclk > ratio from i915->display.cdclk.hw during the pre/post ddb hooks. > cdclk serialization should guard against parallel updates from > two both places and thus isplay.cdclk.hw should be safe to use. > > The other option would be to determine if a cdclk decrease > is going to happen or not, and depending on that use the > old vs. new dbuf_state when updating the ratio in the > pre/post ddb hooks. > > > + new_dbuf_state->joined_mbus); > > +} > > + > > /* > > * Configure MBUS_CTL and all DBUF_CTL_S of each slice to join_mbus state before > > * update the request state of all DBUS slices. > > */ > > -static void update_mbus_pre_enable(struct intel_atomic_state *state) > > +static void intel_dbuf_mbus_join_update(struct intel_atomic_state *state) > > { > > struct drm_i915_private *i915 = to_i915(state->base.dev); > > u32 mbus_ctl; > > - const struct intel_dbuf_state *old_dbuf_state = > > - intel_atomic_get_old_dbuf_state(state); > > const struct intel_dbuf_state *new_dbuf_state = > > intel_atomic_get_new_dbuf_state(state); > > > > @@ -3600,21 +3622,6 @@ static void update_mbus_pre_enable(struct intel_atomic_state *state) > > intel_de_rmw(i915, MBUS_CTL, > > MBUS_HASHING_MODE_MASK | MBUS_JOIN | > > MBUS_JOIN_PIPE_SELECT_MASK, mbus_ctl); > > - > > - if (DISPLAY_VER(i915) >= 20 && > > - old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) { > > - /* > > - * For Xe2LPD and beyond, when there is a change in the ratio > > - * between MDCLK and CDCLK, updates to related registers need to > > - * happen at a specific point in the CDCLK change sequence. In > > - * that case, we defer to the call to > > - * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic. > > - */ > > - return; > > - } > > - > > - intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio, > > - new_dbuf_state->joined_mbus); > > } > > > > void intel_dbuf_pre_plane_update(struct intel_atomic_state *state) > > @@ -3632,7 +3639,11 @@ void intel_dbuf_pre_plane_update(struct intel_atomic_state *state) > > > > WARN_ON(!new_dbuf_state->base.changed); > > > > - update_mbus_pre_enable(state); > > + if (!old_dbuf_state->joined_mbus && new_dbuf_state->joined_mbus) { > > I think you squashed that stuff into the wrong patch. > This one should have a pure refactoring patch. > > > + intel_dbuf_mbus_join_update(state); > > + intel_dbuf_mdclk_min_tracker_update(state); > > + } > > + > > gen9_dbuf_slices_update(i915, > > old_dbuf_state->enabled_slices | > > new_dbuf_state->enabled_slices); > > @@ -3653,6 +3664,11 @@ void intel_dbuf_post_plane_update(struct intel_atomic_state *state) > > > > WARN_ON(!new_dbuf_state->base.changed); > > > > + if (old_dbuf_state->joined_mbus && !new_dbuf_state->joined_mbus) { > > + intel_dbuf_mbus_join_update(state); > > + intel_dbuf_mdclk_min_tracker_update(state); > > + } > > + > > gen9_dbuf_slices_update(i915, > > new_dbuf_state->enabled_slices); > > } > > -- > > 2.37.3 > > -- > Ville Syrjälä > Intel
On Mon, Mar 25, 2024 at 07:01:03PM +0200, Lisovskiy, Stanislav wrote: > On Mon, Mar 25, 2024 at 04:45:49PM +0200, Ville Syrjälä wrote: > > On Mon, Mar 25, 2024 at 01:23:26PM +0200, Stanislav Lisovskiy wrote: > > > According to BSpec we need to do correspondent MBUS updates before > > > or after DBUF reallocation, depending on whether we are enabling > > > or disabling mbus joining(typical scenario is swithing between > > > multiple and single displays). > > > > > > Also we need to be able to update dbuf min tracker and mdclk ratio > > > separately if mbus_join state didn't change, so lets add one > > > degree of freedom and make it possible. > > > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/skl_watermark.c | 54 +++++++++++++------- > > > 1 file changed, 35 insertions(+), 19 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c > > > index bc341abcab2fe..2b947870527fc 100644 > > > --- a/drivers/gpu/drm/i915/display/skl_watermark.c > > > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c > > > @@ -3570,16 +3570,38 @@ void intel_dbuf_mdclk_cdclk_ratio_update(struct drm_i915_private *i915, u8 ratio > > > DBUF_MIN_TRACKER_STATE_SERVICE(ratio - 1)); > > > } > > > > > > +static void intel_dbuf_mdclk_min_tracker_update(struct intel_atomic_state *state) > > > +{ > > > + struct drm_i915_private *i915 = to_i915(state->base.dev); > > > + const struct intel_dbuf_state *old_dbuf_state = > > > + intel_atomic_get_old_dbuf_state(state); > > > + const struct intel_dbuf_state *new_dbuf_state = > > > + intel_atomic_get_new_dbuf_state(state); > > > + > > > + if (DISPLAY_VER(i915) >= 20 && > > > + old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) { > > > + /* > > > + * For Xe2LPD and beyond, when there is a change in the ratio > > > + * between MDCLK and CDCLK, updates to related registers need to > > > + * happen at a specific point in the CDCLK change sequence. In > > > + * that case, we defer to the call to > > > + * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic. > > > + */ > > > + return; > > > + } > > > > That still needs to be removed or else we'll not update the ratio at > > all during the mbus_join changes. I don't think I saw any removal > > in subsequent patches. > > > > > + > > > + intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio, > > I don't get what is happening here. > > "That whole condition I think needs to go. We want to update the ratio > also when changing mbus joining. But that behavioural change doesn't > really belong in this patch, so this is > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>" > > Now it again needs to be changed or changed in other patch(in this series or which one), > I don't follow. > Should it be the patch changing MBUS join value? Yeah, probably should be in the last patch. Perhaps we could change it before that, but that would need some extra brain power to make sure it doesn't temporarily break something. So probably not worth the hassle to do as a separate patch. > > Stan > > > > > And it just occurred to me that this thing will in fact be wrong > > during the pre/post ddb hooks *and* cdclk is getting decreased > > from the post plane update hook. > > > > I can't immediately think of a super nice way to handle this. > > > > Perhaps the most stragithforward idea is to just get the mdclk/cdclk > > ratio from i915->display.cdclk.hw during the pre/post ddb hooks. > > cdclk serialization should guard against parallel updates from > > two both places and thus isplay.cdclk.hw should be safe to use. > > > > The other option would be to determine if a cdclk decrease > > is going to happen or not, and depending on that use the > > old vs. new dbuf_state when updating the ratio in the > > pre/post ddb hooks. > > > > > + new_dbuf_state->joined_mbus); > > > +} > > > + > > > /* > > > * Configure MBUS_CTL and all DBUF_CTL_S of each slice to join_mbus state before > > > * update the request state of all DBUS slices. > > > */ > > > -static void update_mbus_pre_enable(struct intel_atomic_state *state) > > > +static void intel_dbuf_mbus_join_update(struct intel_atomic_state *state) > > > { > > > struct drm_i915_private *i915 = to_i915(state->base.dev); > > > u32 mbus_ctl; > > > - const struct intel_dbuf_state *old_dbuf_state = > > > - intel_atomic_get_old_dbuf_state(state); > > > const struct intel_dbuf_state *new_dbuf_state = > > > intel_atomic_get_new_dbuf_state(state); > > > > > > @@ -3600,21 +3622,6 @@ static void update_mbus_pre_enable(struct intel_atomic_state *state) > > > intel_de_rmw(i915, MBUS_CTL, > > > MBUS_HASHING_MODE_MASK | MBUS_JOIN | > > > MBUS_JOIN_PIPE_SELECT_MASK, mbus_ctl); > > > - > > > - if (DISPLAY_VER(i915) >= 20 && > > > - old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) { > > > - /* > > > - * For Xe2LPD and beyond, when there is a change in the ratio > > > - * between MDCLK and CDCLK, updates to related registers need to > > > - * happen at a specific point in the CDCLK change sequence. In > > > - * that case, we defer to the call to > > > - * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic. > > > - */ > > > - return; > > > - } > > > - > > > - intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio, > > > - new_dbuf_state->joined_mbus); > > > } > > > > > > void intel_dbuf_pre_plane_update(struct intel_atomic_state *state) > > > @@ -3632,7 +3639,11 @@ void intel_dbuf_pre_plane_update(struct intel_atomic_state *state) > > > > > > WARN_ON(!new_dbuf_state->base.changed); > > > > > > - update_mbus_pre_enable(state); > > > + if (!old_dbuf_state->joined_mbus && new_dbuf_state->joined_mbus) { > > > > I think you squashed that stuff into the wrong patch. > > This one should have a pure refactoring patch. > > > > > + intel_dbuf_mbus_join_update(state); > > > + intel_dbuf_mdclk_min_tracker_update(state); > > > + } > > > + > > > gen9_dbuf_slices_update(i915, > > > old_dbuf_state->enabled_slices | > > > new_dbuf_state->enabled_slices); > > > @@ -3653,6 +3664,11 @@ void intel_dbuf_post_plane_update(struct intel_atomic_state *state) > > > > > > WARN_ON(!new_dbuf_state->base.changed); > > > > > > + if (old_dbuf_state->joined_mbus && !new_dbuf_state->joined_mbus) { > > > + intel_dbuf_mbus_join_update(state); > > > + intel_dbuf_mdclk_min_tracker_update(state); > > > + } > > > + > > > gen9_dbuf_slices_update(i915, > > > new_dbuf_state->enabled_slices); > > > } > > > -- > > > 2.37.3 > > > > -- > > Ville Syrjälä > > Intel
On Mon, Mar 25, 2024 at 07:11:21PM +0200, Ville Syrjälä wrote: > On Mon, Mar 25, 2024 at 07:01:03PM +0200, Lisovskiy, Stanislav wrote: > > On Mon, Mar 25, 2024 at 04:45:49PM +0200, Ville Syrjälä wrote: > > > On Mon, Mar 25, 2024 at 01:23:26PM +0200, Stanislav Lisovskiy wrote: > > > > According to BSpec we need to do correspondent MBUS updates before > > > > or after DBUF reallocation, depending on whether we are enabling > > > > or disabling mbus joining(typical scenario is swithing between > > > > multiple and single displays). > > > > > > > > Also we need to be able to update dbuf min tracker and mdclk ratio > > > > separately if mbus_join state didn't change, so lets add one > > > > degree of freedom and make it possible. > > > > > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/skl_watermark.c | 54 +++++++++++++------- > > > > 1 file changed, 35 insertions(+), 19 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c > > > > index bc341abcab2fe..2b947870527fc 100644 > > > > --- a/drivers/gpu/drm/i915/display/skl_watermark.c > > > > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c > > > > @@ -3570,16 +3570,38 @@ void intel_dbuf_mdclk_cdclk_ratio_update(struct drm_i915_private *i915, u8 ratio > > > > DBUF_MIN_TRACKER_STATE_SERVICE(ratio - 1)); > > > > } > > > > > > > > +static void intel_dbuf_mdclk_min_tracker_update(struct intel_atomic_state *state) > > > > +{ > > > > + struct drm_i915_private *i915 = to_i915(state->base.dev); > > > > + const struct intel_dbuf_state *old_dbuf_state = > > > > + intel_atomic_get_old_dbuf_state(state); > > > > + const struct intel_dbuf_state *new_dbuf_state = > > > > + intel_atomic_get_new_dbuf_state(state); > > > > + > > > > + if (DISPLAY_VER(i915) >= 20 && > > > > + old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) { > > > > + /* > > > > + * For Xe2LPD and beyond, when there is a change in the ratio > > > > + * between MDCLK and CDCLK, updates to related registers need to > > > > + * happen at a specific point in the CDCLK change sequence. In > > > > + * that case, we defer to the call to > > > > + * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic. > > > > + */ > > > > + return; > > > > + } > > > > > > That still needs to be removed or else we'll not update the ratio at > > > all during the mbus_join changes. I don't think I saw any removal > > > in subsequent patches. > > > > > > > + > > > > + intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio, > > > > I don't get what is happening here. > > > > "That whole condition I think needs to go. We want to update the ratio > > also when changing mbus joining. But that behavioural change doesn't > > really belong in this patch, so this is > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>" > > > > Now it again needs to be changed or changed in other patch(in this series or which one), > > I don't follow. > > Should it be the patch changing MBUS join value? > > Yeah, probably should be in the last patch. Perhaps we > could change it before that, but that would need some > extra brain power to make sure it doesn't temporarily > break something. So probably not worth the hassle > to do as a separate patch. > > > > > Stan > > > > > > > > And it just occurred to me that this thing will in fact be wrong > > > during the pre/post ddb hooks *and* cdclk is getting decreased > > > from the post plane update hook. > > > > > > I can't immediately think of a super nice way to handle this. First of all why that condition above prevents update when mbus join changes? It exits when mdclk_cdclk ratio is changed not mbus_join? That review process to me seems rather chaotic. Constantly something new pops up, moreover we did previously agree about that code. > > > > > > Perhaps the most stragithforward idea is to just get the mdclk/cdclk > > > ratio from i915->display.cdclk.hw during the pre/post ddb hooks. > > > cdclk serialization should guard against parallel updates from > > > two both places and thus isplay.cdclk.hw should be safe to use. > > > > > > The other option would be to determine if a cdclk decrease > > > is going to happen or not, and depending on that use the > > > old vs. new dbuf_state when updating the ratio in the > > > pre/post ddb hooks. > > > > > > > + new_dbuf_state->joined_mbus); > > > > +} > > > > + > > > > /* > > > > * Configure MBUS_CTL and all DBUF_CTL_S of each slice to join_mbus state before > > > > * update the request state of all DBUS slices. > > > > */ > > > > -static void update_mbus_pre_enable(struct intel_atomic_state *state) > > > > +static void intel_dbuf_mbus_join_update(struct intel_atomic_state *state) > > > > { > > > > struct drm_i915_private *i915 = to_i915(state->base.dev); > > > > u32 mbus_ctl; > > > > - const struct intel_dbuf_state *old_dbuf_state = > > > > - intel_atomic_get_old_dbuf_state(state); > > > > const struct intel_dbuf_state *new_dbuf_state = > > > > intel_atomic_get_new_dbuf_state(state); > > > > > > > > @@ -3600,21 +3622,6 @@ static void update_mbus_pre_enable(struct intel_atomic_state *state) > > > > intel_de_rmw(i915, MBUS_CTL, > > > > MBUS_HASHING_MODE_MASK | MBUS_JOIN | > > > > MBUS_JOIN_PIPE_SELECT_MASK, mbus_ctl); > > > > - > > > > - if (DISPLAY_VER(i915) >= 20 && > > > > - old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) { > > > > - /* > > > > - * For Xe2LPD and beyond, when there is a change in the ratio > > > > - * between MDCLK and CDCLK, updates to related registers need to > > > > - * happen at a specific point in the CDCLK change sequence. In > > > > - * that case, we defer to the call to > > > > - * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic. > > > > - */ > > > > - return; > > > > - } > > > > - > > > > - intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio, > > > > - new_dbuf_state->joined_mbus); > > > > } > > > > > > > > void intel_dbuf_pre_plane_update(struct intel_atomic_state *state) > > > > @@ -3632,7 +3639,11 @@ void intel_dbuf_pre_plane_update(struct intel_atomic_state *state) > > > > > > > > WARN_ON(!new_dbuf_state->base.changed); > > > > > > > > - update_mbus_pre_enable(state); > > > > + if (!old_dbuf_state->joined_mbus && new_dbuf_state->joined_mbus) { > > > > > > I think you squashed that stuff into the wrong patch. > > > This one should have a pure refactoring patch. > > > > > > > + intel_dbuf_mbus_join_update(state); > > > > + intel_dbuf_mdclk_min_tracker_update(state); > > > > + } > > > > + > > > > gen9_dbuf_slices_update(i915, > > > > old_dbuf_state->enabled_slices | > > > > new_dbuf_state->enabled_slices); > > > > @@ -3653,6 +3664,11 @@ void intel_dbuf_post_plane_update(struct intel_atomic_state *state) > > > > > > > > WARN_ON(!new_dbuf_state->base.changed); > > > > > > > > + if (old_dbuf_state->joined_mbus && !new_dbuf_state->joined_mbus) { > > > > + intel_dbuf_mbus_join_update(state); > > > > + intel_dbuf_mdclk_min_tracker_update(state); > > > > + } > > > > + > > > > gen9_dbuf_slices_update(i915, > > > > new_dbuf_state->enabled_slices); > > > > } > > > > -- > > > > 2.37.3 > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel
On Mon, Mar 25, 2024 at 08:29:56PM +0200, Lisovskiy, Stanislav wrote: > On Mon, Mar 25, 2024 at 07:11:21PM +0200, Ville Syrjälä wrote: > > On Mon, Mar 25, 2024 at 07:01:03PM +0200, Lisovskiy, Stanislav wrote: > > > On Mon, Mar 25, 2024 at 04:45:49PM +0200, Ville Syrjälä wrote: > > > > On Mon, Mar 25, 2024 at 01:23:26PM +0200, Stanislav Lisovskiy wrote: > > > > > According to BSpec we need to do correspondent MBUS updates before > > > > > or after DBUF reallocation, depending on whether we are enabling > > > > > or disabling mbus joining(typical scenario is swithing between > > > > > multiple and single displays). > > > > > > > > > > Also we need to be able to update dbuf min tracker and mdclk ratio > > > > > separately if mbus_join state didn't change, so lets add one > > > > > degree of freedom and make it possible. > > > > > > > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/display/skl_watermark.c | 54 +++++++++++++------- > > > > > 1 file changed, 35 insertions(+), 19 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c > > > > > index bc341abcab2fe..2b947870527fc 100644 > > > > > --- a/drivers/gpu/drm/i915/display/skl_watermark.c > > > > > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c > > > > > @@ -3570,16 +3570,38 @@ void intel_dbuf_mdclk_cdclk_ratio_update(struct drm_i915_private *i915, u8 ratio > > > > > DBUF_MIN_TRACKER_STATE_SERVICE(ratio - 1)); > > > > > } > > > > > > > > > > +static void intel_dbuf_mdclk_min_tracker_update(struct intel_atomic_state *state) > > > > > +{ > > > > > + struct drm_i915_private *i915 = to_i915(state->base.dev); > > > > > + const struct intel_dbuf_state *old_dbuf_state = > > > > > + intel_atomic_get_old_dbuf_state(state); > > > > > + const struct intel_dbuf_state *new_dbuf_state = > > > > > + intel_atomic_get_new_dbuf_state(state); > > > > > + > > > > > + if (DISPLAY_VER(i915) >= 20 && > > > > > + old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) { > > > > > + /* > > > > > + * For Xe2LPD and beyond, when there is a change in the ratio > > > > > + * between MDCLK and CDCLK, updates to related registers need to > > > > > + * happen at a specific point in the CDCLK change sequence. In > > > > > + * that case, we defer to the call to > > > > > + * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic. > > > > > + */ > > > > > + return; > > > > > + } > > > > > > > > That still needs to be removed or else we'll not update the ratio at > > > > all during the mbus_join changes. I don't think I saw any removal > > > > in subsequent patches. > > > > > > > > > + > > > > > + intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio, > > > > > > I don't get what is happening here. > > > > > > "That whole condition I think needs to go. We want to update the ratio > > > also when changing mbus joining. But that behavioural change doesn't > > > really belong in this patch, so this is > > > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>" > > > > > > Now it again needs to be changed or changed in other patch(in this series or which one), > > > I don't follow. > > > Should it be the patch changing MBUS join value? > > > > Yeah, probably should be in the last patch. Perhaps we > > could change it before that, but that would need some > > extra brain power to make sure it doesn't temporarily > > break something. So probably not worth the hassle > > to do as a separate patch. > > > > > > > > Stan > > > > > > > > > > > And it just occurred to me that this thing will in fact be wrong > > > > during the pre/post ddb hooks *and* cdclk is getting decreased > > > > from the post plane update hook. > > > > > > > > I can't immediately think of a super nice way to handle this. > > First of all why that > condition above prevents update when mbus join changes? > It exits when mdclk_cdclk ratio is changed not mbus_join? And what happens when mbus_join needs to be changed but mdclk_cdclk_ratio remains unchanged? > > That review process to me seems rather chaotic. > Constantly something new pops up, moreover we did previously agree > about that code. The review process exists to make sure the code actually works correctly. New things come up because of how human brains work, not all things are immediately apparent to everyone. If that were the case then you should have been able to make the code 100% correct from the start, and I wouldn't be able to come up with new ways in which it can fail. So I guess you're the pot and I'm the kettle?
On Mon, Mar 25, 2024 at 08:43:10PM +0200, Ville Syrjälä wrote: > On Mon, Mar 25, 2024 at 08:29:56PM +0200, Lisovskiy, Stanislav wrote: > > On Mon, Mar 25, 2024 at 07:11:21PM +0200, Ville Syrjälä wrote: > > > On Mon, Mar 25, 2024 at 07:01:03PM +0200, Lisovskiy, Stanislav wrote: > > > > On Mon, Mar 25, 2024 at 04:45:49PM +0200, Ville Syrjälä wrote: > > > > > On Mon, Mar 25, 2024 at 01:23:26PM +0200, Stanislav Lisovskiy wrote: > > > > > > According to BSpec we need to do correspondent MBUS updates before > > > > > > or after DBUF reallocation, depending on whether we are enabling > > > > > > or disabling mbus joining(typical scenario is swithing between > > > > > > multiple and single displays). > > > > > > > > > > > > Also we need to be able to update dbuf min tracker and mdclk ratio > > > > > > separately if mbus_join state didn't change, so lets add one > > > > > > degree of freedom and make it possible. > > > > > > > > > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > --- > > > > > > drivers/gpu/drm/i915/display/skl_watermark.c | 54 +++++++++++++------- > > > > > > 1 file changed, 35 insertions(+), 19 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c > > > > > > index bc341abcab2fe..2b947870527fc 100644 > > > > > > --- a/drivers/gpu/drm/i915/display/skl_watermark.c > > > > > > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c > > > > > > @@ -3570,16 +3570,38 @@ void intel_dbuf_mdclk_cdclk_ratio_update(struct drm_i915_private *i915, u8 ratio > > > > > > DBUF_MIN_TRACKER_STATE_SERVICE(ratio - 1)); > > > > > > } > > > > > > > > > > > > +static void intel_dbuf_mdclk_min_tracker_update(struct intel_atomic_state *state) > > > > > > +{ > > > > > > + struct drm_i915_private *i915 = to_i915(state->base.dev); > > > > > > + const struct intel_dbuf_state *old_dbuf_state = > > > > > > + intel_atomic_get_old_dbuf_state(state); > > > > > > + const struct intel_dbuf_state *new_dbuf_state = > > > > > > + intel_atomic_get_new_dbuf_state(state); > > > > > > + > > > > > > + if (DISPLAY_VER(i915) >= 20 && > > > > > > + old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) { > > > > > > + /* > > > > > > + * For Xe2LPD and beyond, when there is a change in the ratio > > > > > > + * between MDCLK and CDCLK, updates to related registers need to > > > > > > + * happen at a specific point in the CDCLK change sequence. In > > > > > > + * that case, we defer to the call to > > > > > > + * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic. > > > > > > + */ > > > > > > + return; > > > > > > + } > > > > > > > > > > That still needs to be removed or else we'll not update the ratio at > > > > > all during the mbus_join changes. I don't think I saw any removal > > > > > in subsequent patches. > > > > > > > > > > > + > > > > > > + intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio, > > > > > > > > I don't get what is happening here. > > > > > > > > "That whole condition I think needs to go. We want to update the ratio > > > > also when changing mbus joining. But that behavioural change doesn't > > > > really belong in this patch, so this is > > > > > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>" > > > > > > > > Now it again needs to be changed or changed in other patch(in this series or which one), > > > > I don't follow. > > > > Should it be the patch changing MBUS join value? > > > > > > Yeah, probably should be in the last patch. Perhaps we > > > could change it before that, but that would need some > > > extra brain power to make sure it doesn't temporarily > > > break something. So probably not worth the hassle > > > to do as a separate patch. > > > > > > > > > > > Stan > > > > > > > > > > > > > > And it just occurred to me that this thing will in fact be wrong > > > > > during the pre/post ddb hooks *and* cdclk is getting decreased > > > > > from the post plane update hook. > > > > > > > > > > I can't immediately think of a super nice way to handle this. > > > > First of all why that > > condition above prevents update when mbus join changes? > > It exits when mdclk_cdclk ratio is changed not mbus_join? > > And what happens when mbus_join needs to be changed > but mdclk_cdclk_ratio remains unchanged? If it is not changed, that condition won't exit, intel_dbuf_mdclk_cdclk_ratio_update will get called. > > > > > That review process to me seems rather chaotic. > > Constantly something new pops up, moreover we did previously agree > > about that code. > > The review process exists to make sure the code actually > works correctly. New things come up because of how human > brains work, not all things are immediately apparent to > everyone. If that were the case then you should have > been able to make the code 100% correct from the start, > and I wouldn't be able to come up with new ways in > which it can fail. So I guess you're the pot and > I'm the kettle? So do you mean that all code that you commit or give r-b doesn't have issue and/or will never be required to improve? There has to be some constructive planning or discussion of what we aim to do at that stage and what is an acceptance criteria. Even google/chrome guys tested initially those patches and were fine with changes. However what I see here is that you are constantly coming up with something new. And both you and me know that current code is far from perfect as well currently, there are still exist unsolved problems. So what? We are anyway constantly improving, but not trying to achieve everything in a single "perfect" patch series. I don't get why this "perfection" is so particularly required from me here. Morever many things have to be done in a way exactly how you say, with no freedom or space for another opinion, while things like whether to use or not additional variable in the code, quite often can be done in multiple ways and it is often quite arguable to say the least, what is the best way to do that. > > -- > Ville Syrjälä > Intel
On Mon, Mar 25, 2024 at 09:03:32PM +0200, Lisovskiy, Stanislav wrote: > On Mon, Mar 25, 2024 at 08:43:10PM +0200, Ville Syrjälä wrote: > > On Mon, Mar 25, 2024 at 08:29:56PM +0200, Lisovskiy, Stanislav wrote: > > > On Mon, Mar 25, 2024 at 07:11:21PM +0200, Ville Syrjälä wrote: > > > > On Mon, Mar 25, 2024 at 07:01:03PM +0200, Lisovskiy, Stanislav wrote: > > > > > On Mon, Mar 25, 2024 at 04:45:49PM +0200, Ville Syrjälä wrote: > > > > > > On Mon, Mar 25, 2024 at 01:23:26PM +0200, Stanislav Lisovskiy wrote: > > > > > > > According to BSpec we need to do correspondent MBUS updates before > > > > > > > or after DBUF reallocation, depending on whether we are enabling > > > > > > > or disabling mbus joining(typical scenario is swithing between > > > > > > > multiple and single displays). > > > > > > > > > > > > > > Also we need to be able to update dbuf min tracker and mdclk ratio > > > > > > > separately if mbus_join state didn't change, so lets add one > > > > > > > degree of freedom and make it possible. > > > > > > > > > > > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > --- > > > > > > > drivers/gpu/drm/i915/display/skl_watermark.c | 54 +++++++++++++------- > > > > > > > 1 file changed, 35 insertions(+), 19 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c > > > > > > > index bc341abcab2fe..2b947870527fc 100644 > > > > > > > --- a/drivers/gpu/drm/i915/display/skl_watermark.c > > > > > > > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c > > > > > > > @@ -3570,16 +3570,38 @@ void intel_dbuf_mdclk_cdclk_ratio_update(struct drm_i915_private *i915, u8 ratio > > > > > > > DBUF_MIN_TRACKER_STATE_SERVICE(ratio - 1)); > > > > > > > } > > > > > > > > > > > > > > +static void intel_dbuf_mdclk_min_tracker_update(struct intel_atomic_state *state) > > > > > > > +{ > > > > > > > + struct drm_i915_private *i915 = to_i915(state->base.dev); > > > > > > > + const struct intel_dbuf_state *old_dbuf_state = > > > > > > > + intel_atomic_get_old_dbuf_state(state); > > > > > > > + const struct intel_dbuf_state *new_dbuf_state = > > > > > > > + intel_atomic_get_new_dbuf_state(state); > > > > > > > + > > > > > > > + if (DISPLAY_VER(i915) >= 20 && > > > > > > > + old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) { > > > > > > > + /* > > > > > > > + * For Xe2LPD and beyond, when there is a change in the ratio > > > > > > > + * between MDCLK and CDCLK, updates to related registers need to > > > > > > > + * happen at a specific point in the CDCLK change sequence. In > > > > > > > + * that case, we defer to the call to > > > > > > > + * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic. > > > > > > > + */ > > > > > > > + return; > > > > > > > + } > > > > > > > > > > > > That still needs to be removed or else we'll not update the ratio at > > > > > > all during the mbus_join changes. I don't think I saw any removal > > > > > > in subsequent patches. > > > > > > > > > > > > > + > > > > > > > + intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio, > > > > > > > > > > I don't get what is happening here. > > > > > > > > > > "That whole condition I think needs to go. We want to update the ratio > > > > > also when changing mbus joining. But that behavioural change doesn't > > > > > really belong in this patch, so this is > > > > > > > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>" > > > > > > > > > > Now it again needs to be changed or changed in other patch(in this series or which one), > > > > > I don't follow. > > > > > Should it be the patch changing MBUS join value? > > > > > > > > Yeah, probably should be in the last patch. Perhaps we > > > > could change it before that, but that would need some > > > > extra brain power to make sure it doesn't temporarily > > > > break something. So probably not worth the hassle > > > > to do as a separate patch. > > > > > > > > > > > > > > Stan > > > > > > > > > > > > > > > > > And it just occurred to me that this thing will in fact be wrong > > > > > > during the pre/post ddb hooks *and* cdclk is getting decreased > > > > > > from the post plane update hook. > > > > > > > > > > > > I can't immediately think of a super nice way to handle this. > > > > > > First of all why that > > > condition above prevents update when mbus join changes? > > > It exits when mdclk_cdclk ratio is changed not mbus_join? > > > > And what happens when mbus_join needs to be changed > > but mdclk_cdclk_ratio remains unchanged? > > If it is not changed, that condition won't exit, > intel_dbuf_mdclk_cdclk_ratio_update will get called. Hmm, right. I read it the wrong way around I guess. But it's still wrong. It will be called only when we change cdclk (and perhaps not always even in that case) not when we change mbus_join. We need to call it in both cases because they happen at different times in the sequence and we want to keep this stuff in sync with the actual hardware state (so same deal as in the the pre plane cdclk hook). > > > > > > > > That review process to me seems rather chaotic. > > > Constantly something new pops up, moreover we did previously agree > > > about that code. > > > > The review process exists to make sure the code actually > > works correctly. New things come up because of how human > > brains work, not all things are immediately apparent to > > everyone. If that were the case then you should have > > been able to make the code 100% correct from the start, > > and I wouldn't be able to come up with new ways in > > which it can fail. So I guess you're the pot and > > I'm the kettle? > > So do you mean that all code that you commit or give r-b > doesn't have issue and/or will never be required to improve? Rb == says what it does on the tin and has no known problems that can cause real problems. So far this does not meet that criteria. It's fine to have eg. known gaps in functionality if we plan on dealing with those later. This is, for example, what we did for the mbus joining originally. But every patch must work correctly regardless of those gaps. Of course we sometimes fail at that and bugs do slip in, but introducing issues on purpose is not acceptable. So we need to make sure the ratio gets correctly programmed in all the steps of the sequence I outlined before. Let me list it here again: 1. disable pipes 2. increase cdclk 2.1 reprogram cdclk 2.2 update dbuf tracker value 3. enable mbus joining if necessary 3.1 update mbus_ctl 3.2 update dbuf tracker value 4. reallocate dbuf for planes on active pipes 5. disable mbus joining if necessary 5.1 update dbuf tracker value 5.2 update mbus_ctl 6. enable pipes 7. decrease cdclk, mbus joining is unchanged 7.1 update dbuf tracker value 7.2 reprogram cdclk And in order to keep things in sync we need: Step 2: - mbus_join == old - mdclk/cdclk ratio == new Step 3: - mbus_join == new - mdclk/cdclk ratio == old when cdclk is changing in step 7 - mdclk/cdclk ratio == new otherwise Step 5: - mbus_join == new - mdclk/cdclk ratio == old when cdclk is changing in step 7 - mdclk/cdclk ratio == new otherwise Step 7: - mbus_join == new - mdclk/cdclk ratio == new > Morever many things have to be done in a way exactly how > you say, with no freedom or space for another opinion, while > things like whether to use or not additional variable in the code, Such things are about making the code: - succinct - easy to read and understand - easy to maintain Unnecessary fluff hinders all that. So just common practice when writing software that needs to stay readable and maintainable.
On Tue, Mar 26, 2024 at 02:12:47PM +0200, Ville Syrjälä wrote: > On Mon, Mar 25, 2024 at 09:03:32PM +0200, Lisovskiy, Stanislav wrote: > > On Mon, Mar 25, 2024 at 08:43:10PM +0200, Ville Syrjälä wrote: > > > On Mon, Mar 25, 2024 at 08:29:56PM +0200, Lisovskiy, Stanislav wrote: > > > > On Mon, Mar 25, 2024 at 07:11:21PM +0200, Ville Syrjälä wrote: > > > > > On Mon, Mar 25, 2024 at 07:01:03PM +0200, Lisovskiy, Stanislav wrote: > > > > > > On Mon, Mar 25, 2024 at 04:45:49PM +0200, Ville Syrjälä wrote: > > > > > > > On Mon, Mar 25, 2024 at 01:23:26PM +0200, Stanislav Lisovskiy wrote: > > > > > > > > According to BSpec we need to do correspondent MBUS updates before > > > > > > > > or after DBUF reallocation, depending on whether we are enabling > > > > > > > > or disabling mbus joining(typical scenario is swithing between > > > > > > > > multiple and single displays). > > > > > > > > > > > > > > > > Also we need to be able to update dbuf min tracker and mdclk ratio > > > > > > > > separately if mbus_join state didn't change, so lets add one > > > > > > > > degree of freedom and make it possible. > > > > > > > > > > > > > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > > --- > > > > > > > > drivers/gpu/drm/i915/display/skl_watermark.c | 54 +++++++++++++------- > > > > > > > > 1 file changed, 35 insertions(+), 19 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c > > > > > > > > index bc341abcab2fe..2b947870527fc 100644 > > > > > > > > --- a/drivers/gpu/drm/i915/display/skl_watermark.c > > > > > > > > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c > > > > > > > > @@ -3570,16 +3570,38 @@ void intel_dbuf_mdclk_cdclk_ratio_update(struct drm_i915_private *i915, u8 ratio > > > > > > > > DBUF_MIN_TRACKER_STATE_SERVICE(ratio - 1)); > > > > > > > > } > > > > > > > > > > > > > > > > +static void intel_dbuf_mdclk_min_tracker_update(struct intel_atomic_state *state) > > > > > > > > +{ > > > > > > > > + struct drm_i915_private *i915 = to_i915(state->base.dev); > > > > > > > > + const struct intel_dbuf_state *old_dbuf_state = > > > > > > > > + intel_atomic_get_old_dbuf_state(state); > > > > > > > > + const struct intel_dbuf_state *new_dbuf_state = > > > > > > > > + intel_atomic_get_new_dbuf_state(state); > > > > > > > > + > > > > > > > > + if (DISPLAY_VER(i915) >= 20 && > > > > > > > > + old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) { > > > > > > > > + /* > > > > > > > > + * For Xe2LPD and beyond, when there is a change in the ratio > > > > > > > > + * between MDCLK and CDCLK, updates to related registers need to > > > > > > > > + * happen at a specific point in the CDCLK change sequence. In > > > > > > > > + * that case, we defer to the call to > > > > > > > > + * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic. > > > > > > > > + */ > > > > > > > > + return; > > > > > > > > + } > > > > > > > > > > > > > > That still needs to be removed or else we'll not update the ratio at > > > > > > > all during the mbus_join changes. I don't think I saw any removal > > > > > > > in subsequent patches. > > > > > > > > > > > > > > > + > > > > > > > > + intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio, > > > > > > > > > > > > I don't get what is happening here. > > > > > > > > > > > > "That whole condition I think needs to go. We want to update the ratio > > > > > > also when changing mbus joining. But that behavioural change doesn't > > > > > > really belong in this patch, so this is > > > > > > > > > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>" > > > > > > > > > > > > Now it again needs to be changed or changed in other patch(in this series or which one), > > > > > > I don't follow. > > > > > > Should it be the patch changing MBUS join value? > > > > > > > > > > Yeah, probably should be in the last patch. Perhaps we > > > > > could change it before that, but that would need some > > > > > extra brain power to make sure it doesn't temporarily > > > > > break something. So probably not worth the hassle > > > > > to do as a separate patch. > > > > > > > > > > > > > > > > > Stan > > > > > > > > > > > > > > > > > > > > And it just occurred to me that this thing will in fact be wrong > > > > > > > during the pre/post ddb hooks *and* cdclk is getting decreased > > > > > > > from the post plane update hook. > > > > > > > > > > > > > > I can't immediately think of a super nice way to handle this. > > > > > > > > First of all why that > > > > condition above prevents update when mbus join changes? > > > > It exits when mdclk_cdclk ratio is changed not mbus_join? > > > > > > And what happens when mbus_join needs to be changed > > > but mdclk_cdclk_ratio remains unchanged? > > > > If it is not changed, that condition won't exit, > > intel_dbuf_mdclk_cdclk_ratio_update will get called. > > Hmm, right. I read it the wrong way around I guess. But it's > still wrong. It will be called only when we change cdclk > (and perhaps not always even in that case) not when we > change mbus_join. We need to call it in both cases because > they happen at different times in the sequence and we > want to keep this stuff in sync with the actual hardware > state (so same deal as in the the pre plane cdclk hook). > > > > > > > > > > > > That review process to me seems rather chaotic. > > > > Constantly something new pops up, moreover we did previously agree > > > > about that code. > > > > > > The review process exists to make sure the code actually > > > works correctly. New things come up because of how human > > > brains work, not all things are immediately apparent to > > > everyone. If that were the case then you should have > > > been able to make the code 100% correct from the start, > > > and I wouldn't be able to come up with new ways in > > > which it can fail. So I guess you're the pot and > > > I'm the kettle? > > > > So do you mean that all code that you commit or give r-b > > doesn't have issue and/or will never be required to improve? > > Rb == says what it does on the tin and has no known > problems that can cause real problems. So far this > does not meet that criteria. > > It's fine to have eg. known gaps in functionality > if we plan on dealing with those later. This is, > for example, what we did for the mbus joining > originally. But every patch must work correctly > regardless of those gaps. Of course we sometimes fail > at that and bugs do slip in, but introducing issues > on purpose is not acceptable. > > So we need to make sure the ratio gets correctly programmed > in all the steps of the sequence I outlined before. Let me > list it here again: > 1. disable pipes > 2. increase cdclk > 2.1 reprogram cdclk > 2.2 update dbuf tracker value > 3. enable mbus joining if necessary > 3.1 update mbus_ctl > 3.2 update dbuf tracker value > 4. reallocate dbuf for planes on active pipes > 5. disable mbus joining if necessary > 5.1 update dbuf tracker value > 5.2 update mbus_ctl > 6. enable pipes > 7. decrease cdclk, mbus joining is unchanged > 7.1 update dbuf tracker value > 7.2 reprogram cdclk Ugh. I had a look at our cdclk pre/post hooks and those actually look very scary. It seems we never updated the logic there to correctly handle crawl/squash. So it will now happily do the cdclk update always from intel_set_cdclk_pre_plane_update() even when decreasing the cdclk frequency. I'll go cook up a patch...
diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c index bc341abcab2fe..2b947870527fc 100644 --- a/drivers/gpu/drm/i915/display/skl_watermark.c +++ b/drivers/gpu/drm/i915/display/skl_watermark.c @@ -3570,16 +3570,38 @@ void intel_dbuf_mdclk_cdclk_ratio_update(struct drm_i915_private *i915, u8 ratio DBUF_MIN_TRACKER_STATE_SERVICE(ratio - 1)); } +static void intel_dbuf_mdclk_min_tracker_update(struct intel_atomic_state *state) +{ + struct drm_i915_private *i915 = to_i915(state->base.dev); + const struct intel_dbuf_state *old_dbuf_state = + intel_atomic_get_old_dbuf_state(state); + const struct intel_dbuf_state *new_dbuf_state = + intel_atomic_get_new_dbuf_state(state); + + if (DISPLAY_VER(i915) >= 20 && + old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) { + /* + * For Xe2LPD and beyond, when there is a change in the ratio + * between MDCLK and CDCLK, updates to related registers need to + * happen at a specific point in the CDCLK change sequence. In + * that case, we defer to the call to + * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic. + */ + return; + } + + intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio, + new_dbuf_state->joined_mbus); +} + /* * Configure MBUS_CTL and all DBUF_CTL_S of each slice to join_mbus state before * update the request state of all DBUS slices. */ -static void update_mbus_pre_enable(struct intel_atomic_state *state) +static void intel_dbuf_mbus_join_update(struct intel_atomic_state *state) { struct drm_i915_private *i915 = to_i915(state->base.dev); u32 mbus_ctl; - const struct intel_dbuf_state *old_dbuf_state = - intel_atomic_get_old_dbuf_state(state); const struct intel_dbuf_state *new_dbuf_state = intel_atomic_get_new_dbuf_state(state); @@ -3600,21 +3622,6 @@ static void update_mbus_pre_enable(struct intel_atomic_state *state) intel_de_rmw(i915, MBUS_CTL, MBUS_HASHING_MODE_MASK | MBUS_JOIN | MBUS_JOIN_PIPE_SELECT_MASK, mbus_ctl); - - if (DISPLAY_VER(i915) >= 20 && - old_dbuf_state->mdclk_cdclk_ratio != new_dbuf_state->mdclk_cdclk_ratio) { - /* - * For Xe2LPD and beyond, when there is a change in the ratio - * between MDCLK and CDCLK, updates to related registers need to - * happen at a specific point in the CDCLK change sequence. In - * that case, we defer to the call to - * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic. - */ - return; - } - - intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio, - new_dbuf_state->joined_mbus); } void intel_dbuf_pre_plane_update(struct intel_atomic_state *state) @@ -3632,7 +3639,11 @@ void intel_dbuf_pre_plane_update(struct intel_atomic_state *state) WARN_ON(!new_dbuf_state->base.changed); - update_mbus_pre_enable(state); + if (!old_dbuf_state->joined_mbus && new_dbuf_state->joined_mbus) { + intel_dbuf_mbus_join_update(state); + intel_dbuf_mdclk_min_tracker_update(state); + } + gen9_dbuf_slices_update(i915, old_dbuf_state->enabled_slices | new_dbuf_state->enabled_slices); @@ -3653,6 +3664,11 @@ void intel_dbuf_post_plane_update(struct intel_atomic_state *state) WARN_ON(!new_dbuf_state->base.changed); + if (old_dbuf_state->joined_mbus && !new_dbuf_state->joined_mbus) { + intel_dbuf_mbus_join_update(state); + intel_dbuf_mdclk_min_tracker_update(state); + } + gen9_dbuf_slices_update(i915, new_dbuf_state->enabled_slices); }