| Message ID | 20200423075902.21892-4-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SAGV support for Gen12+ | expand |
On Thu, Apr 23, 2020 at 10:58:56AM +0300, Stanislav Lisovskiy wrote: > We need to calculate SAGV mask also in a non-modeset > commit, however currently active_pipes are only calculated > for modesets in global atomic state, thus now we will be > tracking those also in bw_state in order to be able to > properly access global data. > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_bw.h | 3 +++ > drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++----- > 2 files changed, 13 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h > index d6df91058223..898b4a85ccab 100644 > --- a/drivers/gpu/drm/i915/display/intel_bw.h > +++ b/drivers/gpu/drm/i915/display/intel_bw.h > @@ -26,6 +26,9 @@ struct intel_bw_state { > > unsigned int data_rate[I915_MAX_PIPES]; > u8 num_active_planes[I915_MAX_PIPES]; > + > + /* bitmask of active pipes */ > + u8 active_pipes; > }; > > #define to_intel_bw_state(x) container_of((x), struct intel_bw_state, base) > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 7e15cf3368ad..f7249bca3f6f 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3874,6 +3874,7 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > struct intel_bw_state *new_bw_state = NULL; > const struct intel_bw_state *old_bw_state = NULL; > int i; > + bool active_pipes_calculated = false; > > for_each_new_intel_crtc_in_state(state, crtc, > new_crtc_state, i) { > @@ -3883,6 +3884,12 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > old_bw_state = intel_atomic_get_old_bw_state(state); > > + if (!active_pipes_calculated) { > + state->active_pipes = new_bw_state->active_pipes = I don't think we should touch state->active_pipes here. > + intel_calc_active_pipes(state, old_bw_state->active_pipes); > + active_pipes_calculated = true; > + } I'd do this after the loop so we don't need this extra boolean. As far as the active_pipes check in intel_crtc_can_enable_sagv(), I think we can pull it out into intel_compute_sagv_mask() so that we do the check after computing the mask. And of course change it to use bw_state->active_pipes instead. We're also going to need to lock_global_state() if bw_state->active_pipes mask changes. > + > if (intel_crtc_can_enable_sagv(new_crtc_state)) > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); > else > @@ -5911,11 +5918,9 @@ skl_compute_wm(struct intel_atomic_state *state) > if (ret) > return ret; > > - if (state->modeset) { > - ret = intel_compute_sagv_mask(state); > - if (ret) > - return ret; > - } > + ret = intel_compute_sagv_mask(state); > + if (ret) > + return ret; We also need to remove the state->modeset checks around sagv_{pre,post}_update(). > > /* > * skl_compute_ddb() will have adjusted the final watermarks > -- > 2.24.1.485.gad05a3d8e5
On Thu, Apr 30, 2020 at 12:21:04PM +0300, Ville Syrjälä wrote: > On Thu, Apr 23, 2020 at 10:58:56AM +0300, Stanislav Lisovskiy wrote: > > We need to calculate SAGV mask also in a non-modeset > > commit, however currently active_pipes are only calculated > > for modesets in global atomic state, thus now we will be > > tracking those also in bw_state in order to be able to > > properly access global data. > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_bw.h | 3 +++ > > drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++----- > > 2 files changed, 13 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h > > index d6df91058223..898b4a85ccab 100644 > > --- a/drivers/gpu/drm/i915/display/intel_bw.h > > +++ b/drivers/gpu/drm/i915/display/intel_bw.h > > @@ -26,6 +26,9 @@ struct intel_bw_state { > > > > unsigned int data_rate[I915_MAX_PIPES]; > > u8 num_active_planes[I915_MAX_PIPES]; > > + > > + /* bitmask of active pipes */ > > + u8 active_pipes; > > }; > > > > #define to_intel_bw_state(x) container_of((x), struct intel_bw_state, base) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index 7e15cf3368ad..f7249bca3f6f 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -3874,6 +3874,7 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > struct intel_bw_state *new_bw_state = NULL; > > const struct intel_bw_state *old_bw_state = NULL; > > int i; > > + bool active_pipes_calculated = false; > > > > for_each_new_intel_crtc_in_state(state, crtc, > > new_crtc_state, i) { > > @@ -3883,6 +3884,12 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > old_bw_state = intel_atomic_get_old_bw_state(state); > > > > + if (!active_pipes_calculated) { > > + state->active_pipes = new_bw_state->active_pipes = > > I don't think we should touch state->active_pipes here. Well, that was my question actually here as well. I understand that changing state->active_pipes here feels like some unneeded side effect, however having state->active_pipes and bw_state->active_pipes going out of sync doesn't sound very attractive to me either. That is why I don't like this idea of duplication at all - having constant need to sync those state, bw_state, cdclk_state, because they all might have different active_pipes now. > > > + intel_calc_active_pipes(state, old_bw_state->active_pipes); > > + active_pipes_calculated = true; > > + } > > I'd do this after the loop so we don't need this extra boolean. As far > as the active_pipes check in intel_crtc_can_enable_sagv(), I think we > can pull it out into intel_compute_sagv_mask() so that we do the check > after computing the mask. And of course change it to use > bw_state->active_pipes instead. intel_crtc_can_enable_sagv is called per crtc - so can't just pull it out, will have to have to cycles then - one will compute bw_state->active_pipes, and another pipe_sagv_mask. > > We're also going to need to lock_global_state() if bw_state->active_pipes > mask changes. Ohh.. right. Stan > > > + > > if (intel_crtc_can_enable_sagv(new_crtc_state)) > > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); > > else > > @@ -5911,11 +5918,9 @@ skl_compute_wm(struct intel_atomic_state *state) > > if (ret) > > return ret; > > > > - if (state->modeset) { > > - ret = intel_compute_sagv_mask(state); > > - if (ret) > > - return ret; > > - } > > + ret = intel_compute_sagv_mask(state); > > + if (ret) > > + return ret; > > We also need to remove the state->modeset checks around > sagv_{pre,post}_update(). > > > > > /* > > * skl_compute_ddb() will have adjusted the final watermarks > > -- > > 2.24.1.485.gad05a3d8e5 > > -- > Ville Syrjälä > Intel
On Thu, Apr 30, 2020 at 01:05:15PM +0300, Lisovskiy, Stanislav wrote: > On Thu, Apr 30, 2020 at 12:21:04PM +0300, Ville Syrjälä wrote: > > On Thu, Apr 23, 2020 at 10:58:56AM +0300, Stanislav Lisovskiy wrote: > > > We need to calculate SAGV mask also in a non-modeset > > > commit, however currently active_pipes are only calculated > > > for modesets in global atomic state, thus now we will be > > > tracking those also in bw_state in order to be able to > > > properly access global data. > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_bw.h | 3 +++ > > > drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++----- > > > 2 files changed, 13 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h > > > index d6df91058223..898b4a85ccab 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_bw.h > > > +++ b/drivers/gpu/drm/i915/display/intel_bw.h > > > @@ -26,6 +26,9 @@ struct intel_bw_state { > > > > > > unsigned int data_rate[I915_MAX_PIPES]; > > > u8 num_active_planes[I915_MAX_PIPES]; > > > + > > > + /* bitmask of active pipes */ > > > + u8 active_pipes; > > > }; > > > > > > #define to_intel_bw_state(x) container_of((x), struct intel_bw_state, base) > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > index 7e15cf3368ad..f7249bca3f6f 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -3874,6 +3874,7 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > struct intel_bw_state *new_bw_state = NULL; > > > const struct intel_bw_state *old_bw_state = NULL; > > > int i; > > > + bool active_pipes_calculated = false; > > > > > > for_each_new_intel_crtc_in_state(state, crtc, > > > new_crtc_state, i) { > > > @@ -3883,6 +3884,12 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > > old_bw_state = intel_atomic_get_old_bw_state(state); > > > > > > + if (!active_pipes_calculated) { > > > + state->active_pipes = new_bw_state->active_pipes = > > > > I don't think we should touch state->active_pipes here. > > Well, that was my question actually here as well. I understand that changing > state->active_pipes here feels like some unneeded side effect, however having > state->active_pipes and bw_state->active_pipes going out of sync doesn't sound > very attractive to me either. That is why I don't like this idea of duplication > at all - having constant need to sync those state, bw_state, cdclk_state, because > they all might have different active_pipes now. Having an out of date active_pipes anywhere would be a bug in that specific code. Also state->active_pipes is definitely going the way of the dodo soon. > > > > > > + intel_calc_active_pipes(state, old_bw_state->active_pipes); > > > + active_pipes_calculated = true; > > > + } > > > > I'd do this after the loop so we don't need this extra boolean. As far > > as the active_pipes check in intel_crtc_can_enable_sagv(), I think we > > can pull it out into intel_compute_sagv_mask() so that we do the check > > after computing the mask. And of course change it to use > > bw_state->active_pipes instead. > > intel_crtc_can_enable_sagv is called per crtc - so can't just pull it out, > will have to have to cycles then - one will compute bw_state->active_pipes, > and another pipe_sagv_mask. Hmm. Actually I think what we should probably do is keep the active_pipes check in intel_can_enable_sagv(). Ie something like this: intel_can_enable_sagv(bw_state) { if (active_pipes && !is_power_of_2(active_pipes)) return false; return sagv_reject != 0; } compute_sagv() { for_each_crtc() { if (crtc_can_sagv()) sagv_reject &= ~pipe; else sagv_reject |= pipe; } active_pipes = calc_active_pipes(); ... lock/serialize etc. } That way we don't have to update sagv_reject at all based on active_pipes. I think that even makes more sense since the active_pipes check is a global thing and not tied to any specific crtc. We can then make the check conditional on pre-icl (or whatever we want) in a later patch. And finally we can remove it altogether in a separate patch, since I don't think we should have to do it on any platform. > > > > > We're also going to need to lock_global_state() if bw_state->active_pipes > > mask changes. > > Ohh.. right. > > > Stan > > > > > > + > > > if (intel_crtc_can_enable_sagv(new_crtc_state)) > > > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); > > > else > > > @@ -5911,11 +5918,9 @@ skl_compute_wm(struct intel_atomic_state *state) > > > if (ret) > > > return ret; > > > > > > - if (state->modeset) { > > > - ret = intel_compute_sagv_mask(state); > > > - if (ret) > > > - return ret; > > > - } > > > + ret = intel_compute_sagv_mask(state); > > > + if (ret) > > > + return ret; > > > > We also need to remove the state->modeset checks around > > sagv_{pre,post}_update(). > > > > > > > > /* > > > * skl_compute_ddb() will have adjusted the final watermarks > > > -- > > > 2.24.1.485.gad05a3d8e5 > > > > -- > > Ville Syrjälä > > Intel
On Thu, Apr 30, 2020 at 01:32:17PM +0300, Ville Syrjälä wrote: > On Thu, Apr 30, 2020 at 01:05:15PM +0300, Lisovskiy, Stanislav wrote: > > On Thu, Apr 30, 2020 at 12:21:04PM +0300, Ville Syrjälä wrote: > > > On Thu, Apr 23, 2020 at 10:58:56AM +0300, Stanislav Lisovskiy wrote: > > > > We need to calculate SAGV mask also in a non-modeset > > > > commit, however currently active_pipes are only calculated > > > > for modesets in global atomic state, thus now we will be > > > > tracking those also in bw_state in order to be able to > > > > properly access global data. > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_bw.h | 3 +++ > > > > drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++----- > > > > 2 files changed, 13 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h > > > > index d6df91058223..898b4a85ccab 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_bw.h > > > > +++ b/drivers/gpu/drm/i915/display/intel_bw.h > > > > @@ -26,6 +26,9 @@ struct intel_bw_state { > > > > > > > > unsigned int data_rate[I915_MAX_PIPES]; > > > > u8 num_active_planes[I915_MAX_PIPES]; > > > > + > > > > + /* bitmask of active pipes */ > > > > + u8 active_pipes; > > > > }; > > > > > > > > #define to_intel_bw_state(x) container_of((x), struct intel_bw_state, base) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > index 7e15cf3368ad..f7249bca3f6f 100644 > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > @@ -3874,6 +3874,7 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > struct intel_bw_state *new_bw_state = NULL; > > > > const struct intel_bw_state *old_bw_state = NULL; > > > > int i; > > > > + bool active_pipes_calculated = false; > > > > > > > > for_each_new_intel_crtc_in_state(state, crtc, > > > > new_crtc_state, i) { > > > > @@ -3883,6 +3884,12 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > > > > old_bw_state = intel_atomic_get_old_bw_state(state); > > > > > > > > + if (!active_pipes_calculated) { > > > > + state->active_pipes = new_bw_state->active_pipes = > > > > > > I don't think we should touch state->active_pipes here. > > > > Well, that was my question actually here as well. I understand that changing > > state->active_pipes here feels like some unneeded side effect, however having > > state->active_pipes and bw_state->active_pipes going out of sync doesn't sound > > very attractive to me either. That is why I don't like this idea of duplication > > at all - having constant need to sync those state, bw_state, cdclk_state, because > > they all might have different active_pipes now. > > Having an out of date active_pipes anywhere would be a bug in that > specific code. Also state->active_pipes is definitely going the way of > the dodo soon. > > > > > > > > > > + intel_calc_active_pipes(state, old_bw_state->active_pipes); > > > > + active_pipes_calculated = true; > > > > + } > > > > > > I'd do this after the loop so we don't need this extra boolean. As far > > > as the active_pipes check in intel_crtc_can_enable_sagv(), I think we > > > can pull it out into intel_compute_sagv_mask() so that we do the check > > > after computing the mask. And of course change it to use > > > bw_state->active_pipes instead. > > > > intel_crtc_can_enable_sagv is called per crtc - so can't just pull it out, > > will have to have to cycles then - one will compute bw_state->active_pipes, > > and another pipe_sagv_mask. > > Hmm. Actually I think what we should probably do is keep the > active_pipes check in intel_can_enable_sagv(). Ie something like this: > > intel_can_enable_sagv(bw_state) { > if (active_pipes && !is_power_of_2(active_pipes)) > return false; > return sagv_reject != 0; > } I need active_pipes check here for skl code only, as it disables SAGV for multipipe scenarios. Adding this here would generalize it for other platforms and we don't want that for ICL+. In fact that is the only reason I need active pipes here - otherwise I think it was even your comment that we actually don't need those here at all, as we just iterate through crtcs in state - pretty clearly remember we discussed this. Just same way how it's done in intel bw check and other places. Stan > > compute_sagv() { > for_each_crtc() { > if (crtc_can_sagv()) > sagv_reject &= ~pipe; > else > sagv_reject |= pipe; > } > > active_pipes = calc_active_pipes(); > > ... lock/serialize etc. > } > > That way we don't have to update sagv_reject at all based on > active_pipes. I think that even makes more sense since the > active_pipes check is a global thing and not tied to any specific > crtc. > > We can then make the check conditional on pre-icl (or whatever we want) > in a later patch. And finally we can remove it altogether in a separate > patch, since I don't think we should have to do it on any platform. > > > > > > > > > We're also going to need to lock_global_state() if bw_state->active_pipes > > > mask changes. > > > > Ohh.. right. > > > > > > Stan > > > > > > > > > + > > > > if (intel_crtc_can_enable_sagv(new_crtc_state)) > > > > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); > > > > else > > > > @@ -5911,11 +5918,9 @@ skl_compute_wm(struct intel_atomic_state *state) > > > > if (ret) > > > > return ret; > > > > > > > > - if (state->modeset) { > > > > - ret = intel_compute_sagv_mask(state); > > > > - if (ret) > > > > - return ret; > > > > - } > > > > + ret = intel_compute_sagv_mask(state); > > > > + if (ret) > > > > + return ret; > > > > > > We also need to remove the state->modeset checks around > > > sagv_{pre,post}_update(). > > > > > > > > > > > /* > > > > * skl_compute_ddb() will have adjusted the final watermarks > > > > -- > > > > 2.24.1.485.gad05a3d8e5 > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel
On Thu, Apr 30, 2020 at 01:47:02PM +0300, Lisovskiy, Stanislav wrote: > On Thu, Apr 30, 2020 at 01:32:17PM +0300, Ville Syrjälä wrote: > > On Thu, Apr 30, 2020 at 01:05:15PM +0300, Lisovskiy, Stanislav wrote: > > > On Thu, Apr 30, 2020 at 12:21:04PM +0300, Ville Syrjälä wrote: > > > > On Thu, Apr 23, 2020 at 10:58:56AM +0300, Stanislav Lisovskiy wrote: > > > > > We need to calculate SAGV mask also in a non-modeset > > > > > commit, however currently active_pipes are only calculated > > > > > for modesets in global atomic state, thus now we will be > > > > > tracking those also in bw_state in order to be able to > > > > > properly access global data. > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/display/intel_bw.h | 3 +++ > > > > > drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++----- > > > > > 2 files changed, 13 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h > > > > > index d6df91058223..898b4a85ccab 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_bw.h > > > > > +++ b/drivers/gpu/drm/i915/display/intel_bw.h > > > > > @@ -26,6 +26,9 @@ struct intel_bw_state { > > > > > > > > > > unsigned int data_rate[I915_MAX_PIPES]; > > > > > u8 num_active_planes[I915_MAX_PIPES]; > > > > > + > > > > > + /* bitmask of active pipes */ > > > > > + u8 active_pipes; > > > > > }; > > > > > > > > > > #define to_intel_bw_state(x) container_of((x), struct intel_bw_state, base) > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > > index 7e15cf3368ad..f7249bca3f6f 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > > @@ -3874,6 +3874,7 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > struct intel_bw_state *new_bw_state = NULL; > > > > > const struct intel_bw_state *old_bw_state = NULL; > > > > > int i; > > > > > + bool active_pipes_calculated = false; > > > > > > > > > > for_each_new_intel_crtc_in_state(state, crtc, > > > > > new_crtc_state, i) { > > > > > @@ -3883,6 +3884,12 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > > > > > > old_bw_state = intel_atomic_get_old_bw_state(state); > > > > > > > > > > + if (!active_pipes_calculated) { > > > > > + state->active_pipes = new_bw_state->active_pipes = > > > > > > > > I don't think we should touch state->active_pipes here. > > > > > > Well, that was my question actually here as well. I understand that changing > > > state->active_pipes here feels like some unneeded side effect, however having > > > state->active_pipes and bw_state->active_pipes going out of sync doesn't sound > > > very attractive to me either. That is why I don't like this idea of duplication > > > at all - having constant need to sync those state, bw_state, cdclk_state, because > > > they all might have different active_pipes now. > > > > Having an out of date active_pipes anywhere would be a bug in that > > specific code. Also state->active_pipes is definitely going the way of > > the dodo soon. > > > > > > > > > > > > > > + intel_calc_active_pipes(state, old_bw_state->active_pipes); > > > > > + active_pipes_calculated = true; > > > > > + } > > > > > > > > I'd do this after the loop so we don't need this extra boolean. As far > > > > as the active_pipes check in intel_crtc_can_enable_sagv(), I think we > > > > can pull it out into intel_compute_sagv_mask() so that we do the check > > > > after computing the mask. And of course change it to use > > > > bw_state->active_pipes instead. > > > > > > intel_crtc_can_enable_sagv is called per crtc - so can't just pull it out, > > > will have to have to cycles then - one will compute bw_state->active_pipes, > > > and another pipe_sagv_mask. > > > > Hmm. Actually I think what we should probably do is keep the > > active_pipes check in intel_can_enable_sagv(). Ie something like this: > > > > intel_can_enable_sagv(bw_state) { > > if (active_pipes && !is_power_of_2(active_pipes)) > > return false; > > return sagv_reject != 0; > > } > > I need active_pipes check here for skl code only, as it disables SAGV for multipipe > scenarios. Adding this here would generalize it for other platforms and we > don't want that for ICL+. Which is why I said "We can then make the check conditional on pre-icl (or whatever we want) in a later patch". Why in a later patch? Because currently the check is unconditional and it's generally a good idea to limit the number of functional changes per patch to a minimum. > > In fact that is the only reason I need active pipes here - otherwise I think > it was even your comment that we actually don't need those here at all, > as we just iterate through crtcs in state - pretty clearly remember we discussed > this. Just same way how it's done in intel bw check and other places. > > Stan > > > > > compute_sagv() { > > for_each_crtc() { > > if (crtc_can_sagv()) > > sagv_reject &= ~pipe; > > else > > sagv_reject |= pipe; > > } > > > > active_pipes = calc_active_pipes(); > > > > ... lock/serialize etc. > > } > > > > That way we don't have to update sagv_reject at all based on > > active_pipes. I think that even makes more sense since the > > active_pipes check is a global thing and not tied to any specific > > crtc. > > > > We can then make the check conditional on pre-icl (or whatever we want) > > in a later patch. And finally we can remove it altogether in a separate > > patch, since I don't think we should have to do it on any platform. > > > > > > > > > > > > > We're also going to need to lock_global_state() if bw_state->active_pipes > > > > mask changes. > > > > > > Ohh.. right. > > > > > > > > > Stan > > > > > > > > > > > > + > > > > > if (intel_crtc_can_enable_sagv(new_crtc_state)) > > > > > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); > > > > > else > > > > > @@ -5911,11 +5918,9 @@ skl_compute_wm(struct intel_atomic_state *state) > > > > > if (ret) > > > > > return ret; > > > > > > > > > > - if (state->modeset) { > > > > > - ret = intel_compute_sagv_mask(state); > > > > > - if (ret) > > > > > - return ret; > > > > > - } > > > > > + ret = intel_compute_sagv_mask(state); > > > > > + if (ret) > > > > > + return ret; > > > > > > > > We also need to remove the state->modeset checks around > > > > sagv_{pre,post}_update(). > > > > > > > > > > > > > > /* > > > > > * skl_compute_ddb() will have adjusted the final watermarks > > > > > -- > > > > > 2.24.1.485.gad05a3d8e5 > > > > > > > > -- > > > > Ville Syrjälä > > > > Intel > > > > -- > > Ville Syrjälä > > Intel
On Thu, Apr 30, 2020 at 01:55:59PM +0300, Ville Syrjälä wrote: > On Thu, Apr 30, 2020 at 01:47:02PM +0300, Lisovskiy, Stanislav wrote: > > On Thu, Apr 30, 2020 at 01:32:17PM +0300, Ville Syrjälä wrote: > > > On Thu, Apr 30, 2020 at 01:05:15PM +0300, Lisovskiy, Stanislav wrote: > > > > On Thu, Apr 30, 2020 at 12:21:04PM +0300, Ville Syrjälä wrote: > > > > > On Thu, Apr 23, 2020 at 10:58:56AM +0300, Stanislav Lisovskiy wrote: > > > > > > We need to calculate SAGV mask also in a non-modeset > > > > > > commit, however currently active_pipes are only calculated > > > > > > for modesets in global atomic state, thus now we will be > > > > > > tracking those also in bw_state in order to be able to > > > > > > properly access global data. > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > --- > > > > > > drivers/gpu/drm/i915/display/intel_bw.h | 3 +++ > > > > > > drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++----- > > > > > > 2 files changed, 13 insertions(+), 5 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h > > > > > > index d6df91058223..898b4a85ccab 100644 > > > > > > --- a/drivers/gpu/drm/i915/display/intel_bw.h > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_bw.h > > > > > > @@ -26,6 +26,9 @@ struct intel_bw_state { > > > > > > > > > > > > unsigned int data_rate[I915_MAX_PIPES]; > > > > > > u8 num_active_planes[I915_MAX_PIPES]; > > > > > > + > > > > > > + /* bitmask of active pipes */ > > > > > > + u8 active_pipes; > > > > > > }; > > > > > > > > > > > > #define to_intel_bw_state(x) container_of((x), struct intel_bw_state, base) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > > > index 7e15cf3368ad..f7249bca3f6f 100644 > > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > > > @@ -3874,6 +3874,7 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > > struct intel_bw_state *new_bw_state = NULL; > > > > > > const struct intel_bw_state *old_bw_state = NULL; > > > > > > int i; > > > > > > + bool active_pipes_calculated = false; > > > > > > > > > > > > for_each_new_intel_crtc_in_state(state, crtc, > > > > > > new_crtc_state, i) { > > > > > > @@ -3883,6 +3884,12 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > > > > > > > > old_bw_state = intel_atomic_get_old_bw_state(state); > > > > > > > > > > > > + if (!active_pipes_calculated) { > > > > > > + state->active_pipes = new_bw_state->active_pipes = > > > > > > > > > > I don't think we should touch state->active_pipes here. > > > > > > > > Well, that was my question actually here as well. I understand that changing > > > > state->active_pipes here feels like some unneeded side effect, however having > > > > state->active_pipes and bw_state->active_pipes going out of sync doesn't sound > > > > very attractive to me either. That is why I don't like this idea of duplication > > > > at all - having constant need to sync those state, bw_state, cdclk_state, because > > > > they all might have different active_pipes now. > > > > > > Having an out of date active_pipes anywhere would be a bug in that > > > specific code. Also state->active_pipes is definitely going the way of > > > the dodo soon. > > > > > > > > > > > > > > > > > > + intel_calc_active_pipes(state, old_bw_state->active_pipes); > > > > > > + active_pipes_calculated = true; > > > > > > + } > > > > > > > > > > I'd do this after the loop so we don't need this extra boolean. As far > > > > > as the active_pipes check in intel_crtc_can_enable_sagv(), I think we > > > > > can pull it out into intel_compute_sagv_mask() so that we do the check > > > > > after computing the mask. And of course change it to use > > > > > bw_state->active_pipes instead. > > > > > > > > intel_crtc_can_enable_sagv is called per crtc - so can't just pull it out, > > > > will have to have to cycles then - one will compute bw_state->active_pipes, > > > > and another pipe_sagv_mask. > > > > > > Hmm. Actually I think what we should probably do is keep the > > > active_pipes check in intel_can_enable_sagv(). Ie something like this: > > > > > > intel_can_enable_sagv(bw_state) { > > > if (active_pipes && !is_power_of_2(active_pipes)) > > > return false; > > > return sagv_reject != 0; > > > } > > > > I need active_pipes check here for skl code only, as it disables SAGV for multipipe > > scenarios. Adding this here would generalize it for other platforms and we > > don't want that for ICL+. > > Which is why I said "We can then make the check conditional on pre-icl > (or whatever we want) in a later patch". Why in a later patch? Because > currently the check is unconditional and it's generally a good idea to > limit the number of functional changes per patch to a minimum. Moving active_pipes check out of intel_crtc_can_enable_sagv will result in wrong SAGV mask calculated. i.e if you have 2 pipes, for_each_crtc() { if (crtc_can_sagv()) sagv_reject &= ~pipe; else sagv_reject |= pipe; } will calculate sagv_reject as 0 which is wrong and value will be stored in global state. I think active_pipes should always affect the SAGV mask otherwise we do get really strange situation: you have SAGV mask as 0, but you still reject SAGV. So there is no way even then to track what was the previous SAGV state - even if it's 0 it could have been rejected. IMO that is quite weird side effect. So removing active_pipes from intel_crtc_can_enable_sagv doesn't sound like good idea. I think it is now just a bit too much hassle around simple active_pipes_calculated boolean check. Stan > > > > > In fact that is the only reason I need active pipes here - otherwise I think > > it was even your comment that we actually don't need those here at all, > > as we just iterate through crtcs in state - pretty clearly remember we discussed > > this. Just same way how it's done in intel bw check and other places. > > > > Stan > > > > > > > > compute_sagv() { > > > for_each_crtc() { > > > if (crtc_can_sagv()) > > > sagv_reject &= ~pipe; > > > else > > > sagv_reject |= pipe; > > > } > > > > > > active_pipes = calc_active_pipes(); > > > > > > ... lock/serialize etc. > > > } > > > > > > That way we don't have to update sagv_reject at all based on > > > active_pipes. I think that even makes more sense since the > > > active_pipes check is a global thing and not tied to any specific > > > crtc. > > > > > > We can then make the check conditional on pre-icl (or whatever we want) > > > in a later patch. And finally we can remove it altogether in a separate > > > patch, since I don't think we should have to do it on any platform. > > > > > > > > > > > > > > > > > We're also going to need to lock_global_state() if bw_state->active_pipes > > > > > mask changes. > > > > > > > > Ohh.. right. > > > > > > > > > > > > Stan > > > > > > > > > > > > > > > + > > > > > > if (intel_crtc_can_enable_sagv(new_crtc_state)) > > > > > > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); > > > > > > else > > > > > > @@ -5911,11 +5918,9 @@ skl_compute_wm(struct intel_atomic_state *state) > > > > > > if (ret) > > > > > > return ret; > > > > > > > > > > > > - if (state->modeset) { > > > > > > - ret = intel_compute_sagv_mask(state); > > > > > > - if (ret) > > > > > > - return ret; > > > > > > - } > > > > > > + ret = intel_compute_sagv_mask(state); > > > > > > + if (ret) > > > > > > + return ret; > > > > > > > > > > We also need to remove the state->modeset checks around > > > > > sagv_{pre,post}_update(). > > > > > > > > > > > > > > > > > /* > > > > > > * skl_compute_ddb() will have adjusted the final watermarks > > > > > > -- > > > > > > 2.24.1.485.gad05a3d8e5 > > > > > > > > > > -- > > > > > Ville Syrjälä > > > > > Intel > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel
On Thu, Apr 30, 2020 at 02:07:02PM +0300, Lisovskiy, Stanislav wrote: > On Thu, Apr 30, 2020 at 01:55:59PM +0300, Ville Syrjälä wrote: > > On Thu, Apr 30, 2020 at 01:47:02PM +0300, Lisovskiy, Stanislav wrote: > > > On Thu, Apr 30, 2020 at 01:32:17PM +0300, Ville Syrjälä wrote: > > > > On Thu, Apr 30, 2020 at 01:05:15PM +0300, Lisovskiy, Stanislav wrote: > > > > > On Thu, Apr 30, 2020 at 12:21:04PM +0300, Ville Syrjälä wrote: > > > > > > On Thu, Apr 23, 2020 at 10:58:56AM +0300, Stanislav Lisovskiy wrote: > > > > > > > We need to calculate SAGV mask also in a non-modeset > > > > > > > commit, however currently active_pipes are only calculated > > > > > > > for modesets in global atomic state, thus now we will be > > > > > > > tracking those also in bw_state in order to be able to > > > > > > > properly access global data. > > > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > --- > > > > > > > drivers/gpu/drm/i915/display/intel_bw.h | 3 +++ > > > > > > > drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++----- > > > > > > > 2 files changed, 13 insertions(+), 5 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h > > > > > > > index d6df91058223..898b4a85ccab 100644 > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_bw.h > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_bw.h > > > > > > > @@ -26,6 +26,9 @@ struct intel_bw_state { > > > > > > > > > > > > > > unsigned int data_rate[I915_MAX_PIPES]; > > > > > > > u8 num_active_planes[I915_MAX_PIPES]; > > > > > > > + > > > > > > > + /* bitmask of active pipes */ > > > > > > > + u8 active_pipes; > > > > > > > }; > > > > > > > > > > > > > > #define to_intel_bw_state(x) container_of((x), struct intel_bw_state, base) > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > > > > index 7e15cf3368ad..f7249bca3f6f 100644 > > > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > > > > @@ -3874,6 +3874,7 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > > > struct intel_bw_state *new_bw_state = NULL; > > > > > > > const struct intel_bw_state *old_bw_state = NULL; > > > > > > > int i; > > > > > > > + bool active_pipes_calculated = false; > > > > > > > > > > > > > > for_each_new_intel_crtc_in_state(state, crtc, > > > > > > > new_crtc_state, i) { > > > > > > > @@ -3883,6 +3884,12 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > > > > > > > > > > old_bw_state = intel_atomic_get_old_bw_state(state); > > > > > > > > > > > > > > + if (!active_pipes_calculated) { > > > > > > > + state->active_pipes = new_bw_state->active_pipes = > > > > > > > > > > > > I don't think we should touch state->active_pipes here. > > > > > > > > > > Well, that was my question actually here as well. I understand that changing > > > > > state->active_pipes here feels like some unneeded side effect, however having > > > > > state->active_pipes and bw_state->active_pipes going out of sync doesn't sound > > > > > very attractive to me either. That is why I don't like this idea of duplication > > > > > at all - having constant need to sync those state, bw_state, cdclk_state, because > > > > > they all might have different active_pipes now. > > > > > > > > Having an out of date active_pipes anywhere would be a bug in that > > > > specific code. Also state->active_pipes is definitely going the way of > > > > the dodo soon. > > > > > > > > > > > > > > > > > > > > > > + intel_calc_active_pipes(state, old_bw_state->active_pipes); > > > > > > > + active_pipes_calculated = true; > > > > > > > + } > > > > > > > > > > > > I'd do this after the loop so we don't need this extra boolean. As far > > > > > > as the active_pipes check in intel_crtc_can_enable_sagv(), I think we > > > > > > can pull it out into intel_compute_sagv_mask() so that we do the check > > > > > > after computing the mask. And of course change it to use > > > > > > bw_state->active_pipes instead. > > > > > > > > > > intel_crtc_can_enable_sagv is called per crtc - so can't just pull it out, > > > > > will have to have to cycles then - one will compute bw_state->active_pipes, > > > > > and another pipe_sagv_mask. > > > > > > > > Hmm. Actually I think what we should probably do is keep the > > > > active_pipes check in intel_can_enable_sagv(). Ie something like this: > > > > > > > > intel_can_enable_sagv(bw_state) { > > > > if (active_pipes && !is_power_of_2(active_pipes)) > > > > return false; > > > > return sagv_reject != 0; > > > > } > > > > > > I need active_pipes check here for skl code only, as it disables SAGV for multipipe > > > scenarios. Adding this here would generalize it for other platforms and we > > > don't want that for ICL+. > > > > Which is why I said "We can then make the check conditional on pre-icl > > (or whatever we want) in a later patch". Why in a later patch? Because > > currently the check is unconditional and it's generally a good idea to > > limit the number of functional changes per patch to a minimum. > > Moving active_pipes check out of intel_crtc_can_enable_sagv will result > in wrong SAGV mask calculated. > > i.e if you have 2 pipes, > > for_each_crtc() { > if (crtc_can_sagv()) > sagv_reject &= ~pipe; > else > sagv_reject |= pipe; > } > > will calculate sagv_reject as 0 which is wrong and value will be stored > in global state. No, it accurately reflects whether each of those pipes is capable of sagv. The single pipe restriction is an additinal constrain on top of that. In fact I think adjusting sagv_reject_mask based on this would result in the wrong value potentially. Consider for example: 1. Enable pipe A + B 2. sagv_reject would be calculated as 0x3 3. Disable pipe B 4. sagv_reject will have pipe B removed, leaving its value at 0x1 5. No SAGV even though we only have one pipe enabled, which is wrong > I think active_pipes should always affect the SAGV mask > otherwise we do get really strange situation: you have SAGV mask as 0, > but you still reject SAGV. So there is no way even then to track what > was the previous SAGV state - even if it's 0 it could have been rejected. > > IMO that is quite weird side effect. So removing active_pipes from > intel_crtc_can_enable_sagv doesn't sound like good idea. > > I think it is now just a bit too much hassle around simple > active_pipes_calculated boolean check. > > Stan > > > > > > > > > In fact that is the only reason I need active pipes here - otherwise I think > > > it was even your comment that we actually don't need those here at all, > > > as we just iterate through crtcs in state - pretty clearly remember we discussed > > > this. Just same way how it's done in intel bw check and other places. > > > > > > Stan > > > > > > > > > > > compute_sagv() { > > > > for_each_crtc() { > > > > if (crtc_can_sagv()) > > > > sagv_reject &= ~pipe; > > > > else > > > > sagv_reject |= pipe; > > > > } > > > > > > > > active_pipes = calc_active_pipes(); > > > > > > > > ... lock/serialize etc. > > > > } > > > > > > > > That way we don't have to update sagv_reject at all based on > > > > active_pipes. I think that even makes more sense since the > > > > active_pipes check is a global thing and not tied to any specific > > > > crtc. > > > > > > > > We can then make the check conditional on pre-icl (or whatever we want) > > > > in a later patch. And finally we can remove it altogether in a separate > > > > patch, since I don't think we should have to do it on any platform. > > > > > > > > > > > > > > > > > > > > > We're also going to need to lock_global_state() if bw_state->active_pipes > > > > > > mask changes. > > > > > > > > > > Ohh.. right. > > > > > > > > > > > > > > > Stan > > > > > > > > > > > > > > > > > > + > > > > > > > if (intel_crtc_can_enable_sagv(new_crtc_state)) > > > > > > > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); > > > > > > > else > > > > > > > @@ -5911,11 +5918,9 @@ skl_compute_wm(struct intel_atomic_state *state) > > > > > > > if (ret) > > > > > > > return ret; > > > > > > > > > > > > > > - if (state->modeset) { > > > > > > > - ret = intel_compute_sagv_mask(state); > > > > > > > - if (ret) > > > > > > > - return ret; > > > > > > > - } > > > > > > > + ret = intel_compute_sagv_mask(state); > > > > > > > + if (ret) > > > > > > > + return ret; > > > > > > > > > > > > We also need to remove the state->modeset checks around > > > > > > sagv_{pre,post}_update(). > > > > > > > > > > > > > > > > > > > > /* > > > > > > > * skl_compute_ddb() will have adjusted the final watermarks > > > > > > > -- > > > > > > > 2.24.1.485.gad05a3d8e5 > > > > > > > > > > > > -- > > > > > > Ville Syrjälä > > > > > > Intel > > > > > > > > -- > > > > Ville Syrjälä > > > > Intel > > > > -- > > Ville Syrjälä > > Intel
On Thu, Apr 30, 2020 at 02:22:02PM +0300, Ville Syrjälä wrote: > On Thu, Apr 30, 2020 at 02:07:02PM +0300, Lisovskiy, Stanislav wrote: > > On Thu, Apr 30, 2020 at 01:55:59PM +0300, Ville Syrjälä wrote: > > > On Thu, Apr 30, 2020 at 01:47:02PM +0300, Lisovskiy, Stanislav wrote: > > > > On Thu, Apr 30, 2020 at 01:32:17PM +0300, Ville Syrjälä wrote: > > > > > On Thu, Apr 30, 2020 at 01:05:15PM +0300, Lisovskiy, Stanislav wrote: > > > > > > On Thu, Apr 30, 2020 at 12:21:04PM +0300, Ville Syrjälä wrote: > > > > > > > On Thu, Apr 23, 2020 at 10:58:56AM +0300, Stanislav Lisovskiy wrote: > > > > > > > > We need to calculate SAGV mask also in a non-modeset > > > > > > > > commit, however currently active_pipes are only calculated > > > > > > > > for modesets in global atomic state, thus now we will be > > > > > > > > tracking those also in bw_state in order to be able to > > > > > > > > properly access global data. > > > > > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > > --- > > > > > > > > drivers/gpu/drm/i915/display/intel_bw.h | 3 +++ > > > > > > > > drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++----- > > > > > > > > 2 files changed, 13 insertions(+), 5 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h > > > > > > > > index d6df91058223..898b4a85ccab 100644 > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_bw.h > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_bw.h > > > > > > > > @@ -26,6 +26,9 @@ struct intel_bw_state { > > > > > > > > > > > > > > > > unsigned int data_rate[I915_MAX_PIPES]; > > > > > > > > u8 num_active_planes[I915_MAX_PIPES]; > > > > > > > > + > > > > > > > > + /* bitmask of active pipes */ > > > > > > > > + u8 active_pipes; > > > > > > > > }; > > > > > > > > > > > > > > > > #define to_intel_bw_state(x) container_of((x), struct intel_bw_state, base) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > > > > > index 7e15cf3368ad..f7249bca3f6f 100644 > > > > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > > > > > @@ -3874,6 +3874,7 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > > > > struct intel_bw_state *new_bw_state = NULL; > > > > > > > > const struct intel_bw_state *old_bw_state = NULL; > > > > > > > > int i; > > > > > > > > + bool active_pipes_calculated = false; > > > > > > > > > > > > > > > > for_each_new_intel_crtc_in_state(state, crtc, > > > > > > > > new_crtc_state, i) { > > > > > > > > @@ -3883,6 +3884,12 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > > > > > > > > > > > > old_bw_state = intel_atomic_get_old_bw_state(state); > > > > > > > > > > > > > > > > + if (!active_pipes_calculated) { > > > > > > > > + state->active_pipes = new_bw_state->active_pipes = > > > > > > > > > > > > > > I don't think we should touch state->active_pipes here. > > > > > > > > > > > > Well, that was my question actually here as well. I understand that changing > > > > > > state->active_pipes here feels like some unneeded side effect, however having > > > > > > state->active_pipes and bw_state->active_pipes going out of sync doesn't sound > > > > > > very attractive to me either. That is why I don't like this idea of duplication > > > > > > at all - having constant need to sync those state, bw_state, cdclk_state, because > > > > > > they all might have different active_pipes now. > > > > > > > > > > Having an out of date active_pipes anywhere would be a bug in that > > > > > specific code. Also state->active_pipes is definitely going the way of > > > > > the dodo soon. > > > > > > > > > > > > > > > > > > > > > > > > > > + intel_calc_active_pipes(state, old_bw_state->active_pipes); > > > > > > > > + active_pipes_calculated = true; > > > > > > > > + } > > > > > > > > > > > > > > I'd do this after the loop so we don't need this extra boolean. As far > > > > > > > as the active_pipes check in intel_crtc_can_enable_sagv(), I think we > > > > > > > can pull it out into intel_compute_sagv_mask() so that we do the check > > > > > > > after computing the mask. And of course change it to use > > > > > > > bw_state->active_pipes instead. > > > > > > > > > > > > intel_crtc_can_enable_sagv is called per crtc - so can't just pull it out, > > > > > > will have to have to cycles then - one will compute bw_state->active_pipes, > > > > > > and another pipe_sagv_mask. > > > > > > > > > > Hmm. Actually I think what we should probably do is keep the > > > > > active_pipes check in intel_can_enable_sagv(). Ie something like this: > > > > > > > > > > intel_can_enable_sagv(bw_state) { > > > > > if (active_pipes && !is_power_of_2(active_pipes)) > > > > > return false; > > > > > return sagv_reject != 0; > > > > > } > > > > > > > > I need active_pipes check here for skl code only, as it disables SAGV for multipipe > > > > scenarios. Adding this here would generalize it for other platforms and we > > > > don't want that for ICL+. > > > > > > Which is why I said "We can then make the check conditional on pre-icl > > > (or whatever we want) in a later patch". Why in a later patch? Because > > > currently the check is unconditional and it's generally a good idea to > > > limit the number of functional changes per patch to a minimum. > > > > Moving active_pipes check out of intel_crtc_can_enable_sagv will result > > in wrong SAGV mask calculated. > > > > i.e if you have 2 pipes, > > > > for_each_crtc() { > > if (crtc_can_sagv()) > > sagv_reject &= ~pipe; > > else > > sagv_reject |= pipe; > > } > > > > will calculate sagv_reject as 0 which is wrong and value will be stored > > in global state. > > No, it accurately reflects whether each of those pipes is capable of > sagv. The single pipe restriction is an additinal constrain on top of > that. In fact I think adjusting sagv_reject_mask based on this would > result in the wrong value potentially. Consider for example: > > 1. Enable pipe A + B > 2. sagv_reject would be calculated as 0x3 > 3. Disable pipe B > 4. sagv_reject will have pipe B removed, leaving its value at 0x1 > 5. No SAGV even though we only have one pipe enabled, which is wrong Quite good example. I think it means that pipe_sagv_mask should be affected not only by per crtc checks, but also by overall active_pipes state then. I.e ok, I remove active_pipes from intel_crtc_can_enable_sagv, however that active_pipes && !is_power_of_2(active_pipes) check should then also assign pipe_sagv_reject mask to ~0, if it evaluates to true. I.e then we will always have intel_can_enable_sagv result reflected in sagv mask, which is handy, also intel_can_enable_sagv would still just evaluate sagv mask. Stan > > > I think active_pipes should always affect the SAGV mask > > otherwise we do get really strange situation: you have SAGV mask as 0, > > but you still reject SAGV. So there is no way even then to track what > > was the previous SAGV state - even if it's 0 it could have been rejected. > > > > IMO that is quite weird side effect. So removing active_pipes from > > intel_crtc_can_enable_sagv doesn't sound like good idea. > > > > I think it is now just a bit too much hassle around simple > > active_pipes_calculated boolean check. > > > > Stan > > > > > > > > > > > > > In fact that is the only reason I need active pipes here - otherwise I think > > > > it was even your comment that we actually don't need those here at all, > > > > as we just iterate through crtcs in state - pretty clearly remember we discussed > > > > this. Just same way how it's done in intel bw check and other places. > > > > > > > > Stan > > > > > > > > > > > > > > compute_sagv() { > > > > > for_each_crtc() { > > > > > if (crtc_can_sagv()) > > > > > sagv_reject &= ~pipe; > > > > > else > > > > > sagv_reject |= pipe; > > > > > } > > > > > > > > > > active_pipes = calc_active_pipes(); > > > > > > > > > > ... lock/serialize etc. > > > > > } > > > > > > > > > > That way we don't have to update sagv_reject at all based on > > > > > active_pipes. I think that even makes more sense since the > > > > > active_pipes check is a global thing and not tied to any specific > > > > > crtc. > > > > > > > > > > We can then make the check conditional on pre-icl (or whatever we want) > > > > > in a later patch. And finally we can remove it altogether in a separate > > > > > patch, since I don't think we should have to do it on any platform. > > > > > > > > > > > > > > > > > > > > > > > > > We're also going to need to lock_global_state() if bw_state->active_pipes > > > > > > > mask changes. > > > > > > > > > > > > Ohh.. right. > > > > > > > > > > > > > > > > > > Stan > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > if (intel_crtc_can_enable_sagv(new_crtc_state)) > > > > > > > > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); > > > > > > > > else > > > > > > > > @@ -5911,11 +5918,9 @@ skl_compute_wm(struct intel_atomic_state *state) > > > > > > > > if (ret) > > > > > > > > return ret; > > > > > > > > > > > > > > > > - if (state->modeset) { > > > > > > > > - ret = intel_compute_sagv_mask(state); > > > > > > > > - if (ret) > > > > > > > > - return ret; > > > > > > > > - } > > > > > > > > + ret = intel_compute_sagv_mask(state); > > > > > > > > + if (ret) > > > > > > > > + return ret; > > > > > > > > > > > > > > We also need to remove the state->modeset checks around > > > > > > > sagv_{pre,post}_update(). > > > > > > > > > > > > > > > > > > > > > > > /* > > > > > > > > * skl_compute_ddb() will have adjusted the final watermarks > > > > > > > > -- > > > > > > > > 2.24.1.485.gad05a3d8e5 > > > > > > > > > > > > > > -- > > > > > > > Ville Syrjälä > > > > > > > Intel > > > > > > > > > > -- > > > > > Ville Syrjälä > > > > > Intel > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel
On Thu, Apr 30, 2020 at 02:29:51PM +0300, Lisovskiy, Stanislav wrote: > On Thu, Apr 30, 2020 at 02:22:02PM +0300, Ville Syrjälä wrote: > > On Thu, Apr 30, 2020 at 02:07:02PM +0300, Lisovskiy, Stanislav wrote: > > > On Thu, Apr 30, 2020 at 01:55:59PM +0300, Ville Syrjälä wrote: > > > > On Thu, Apr 30, 2020 at 01:47:02PM +0300, Lisovskiy, Stanislav wrote: > > > > > On Thu, Apr 30, 2020 at 01:32:17PM +0300, Ville Syrjälä wrote: > > > > > > On Thu, Apr 30, 2020 at 01:05:15PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > On Thu, Apr 30, 2020 at 12:21:04PM +0300, Ville Syrjälä wrote: > > > > > > > > On Thu, Apr 23, 2020 at 10:58:56AM +0300, Stanislav Lisovskiy wrote: > > > > > > > > > We need to calculate SAGV mask also in a non-modeset > > > > > > > > > commit, however currently active_pipes are only calculated > > > > > > > > > for modesets in global atomic state, thus now we will be > > > > > > > > > tracking those also in bw_state in order to be able to > > > > > > > > > properly access global data. > > > > > > > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > > > --- > > > > > > > > > drivers/gpu/drm/i915/display/intel_bw.h | 3 +++ > > > > > > > > > drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++----- > > > > > > > > > 2 files changed, 13 insertions(+), 5 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h > > > > > > > > > index d6df91058223..898b4a85ccab 100644 > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_bw.h > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_bw.h > > > > > > > > > @@ -26,6 +26,9 @@ struct intel_bw_state { > > > > > > > > > > > > > > > > > > unsigned int data_rate[I915_MAX_PIPES]; > > > > > > > > > u8 num_active_planes[I915_MAX_PIPES]; > > > > > > > > > + > > > > > > > > > + /* bitmask of active pipes */ > > > > > > > > > + u8 active_pipes; > > > > > > > > > }; > > > > > > > > > > > > > > > > > > #define to_intel_bw_state(x) container_of((x), struct intel_bw_state, base) > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > > > > > > index 7e15cf3368ad..f7249bca3f6f 100644 > > > > > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > > > > > > @@ -3874,6 +3874,7 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > > > > > struct intel_bw_state *new_bw_state = NULL; > > > > > > > > > const struct intel_bw_state *old_bw_state = NULL; > > > > > > > > > int i; > > > > > > > > > + bool active_pipes_calculated = false; > > > > > > > > > > > > > > > > > > for_each_new_intel_crtc_in_state(state, crtc, > > > > > > > > > new_crtc_state, i) { > > > > > > > > > @@ -3883,6 +3884,12 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > > > > > > > > > > > > > > old_bw_state = intel_atomic_get_old_bw_state(state); > > > > > > > > > > > > > > > > > > + if (!active_pipes_calculated) { > > > > > > > > > + state->active_pipes = new_bw_state->active_pipes = > > > > > > > > > > > > > > > > I don't think we should touch state->active_pipes here. > > > > > > > > > > > > > > Well, that was my question actually here as well. I understand that changing > > > > > > > state->active_pipes here feels like some unneeded side effect, however having > > > > > > > state->active_pipes and bw_state->active_pipes going out of sync doesn't sound > > > > > > > very attractive to me either. That is why I don't like this idea of duplication > > > > > > > at all - having constant need to sync those state, bw_state, cdclk_state, because > > > > > > > they all might have different active_pipes now. > > > > > > > > > > > > Having an out of date active_pipes anywhere would be a bug in that > > > > > > specific code. Also state->active_pipes is definitely going the way of > > > > > > the dodo soon. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + intel_calc_active_pipes(state, old_bw_state->active_pipes); > > > > > > > > > + active_pipes_calculated = true; > > > > > > > > > + } > > > > > > > > > > > > > > > > I'd do this after the loop so we don't need this extra boolean. As far > > > > > > > > as the active_pipes check in intel_crtc_can_enable_sagv(), I think we > > > > > > > > can pull it out into intel_compute_sagv_mask() so that we do the check > > > > > > > > after computing the mask. And of course change it to use > > > > > > > > bw_state->active_pipes instead. > > > > > > > > > > > > > > intel_crtc_can_enable_sagv is called per crtc - so can't just pull it out, > > > > > > > will have to have to cycles then - one will compute bw_state->active_pipes, > > > > > > > and another pipe_sagv_mask. > > > > > > > > > > > > Hmm. Actually I think what we should probably do is keep the > > > > > > active_pipes check in intel_can_enable_sagv(). Ie something like this: > > > > > > > > > > > > intel_can_enable_sagv(bw_state) { > > > > > > if (active_pipes && !is_power_of_2(active_pipes)) > > > > > > return false; > > > > > > return sagv_reject != 0; > > > > > > } > > > > > > > > > > I need active_pipes check here for skl code only, as it disables SAGV for multipipe > > > > > scenarios. Adding this here would generalize it for other platforms and we > > > > > don't want that for ICL+. > > > > > > > > Which is why I said "We can then make the check conditional on pre-icl > > > > (or whatever we want) in a later patch". Why in a later patch? Because > > > > currently the check is unconditional and it's generally a good idea to > > > > limit the number of functional changes per patch to a minimum. > > > > > > Moving active_pipes check out of intel_crtc_can_enable_sagv will result > > > in wrong SAGV mask calculated. > > > > > > i.e if you have 2 pipes, > > > > > > for_each_crtc() { > > > if (crtc_can_sagv()) > > > sagv_reject &= ~pipe; > > > else > > > sagv_reject |= pipe; > > > } > > > > > > will calculate sagv_reject as 0 which is wrong and value will be stored > > > in global state. > > > > No, it accurately reflects whether each of those pipes is capable of > > sagv. The single pipe restriction is an additinal constrain on top of > > that. In fact I think adjusting sagv_reject_mask based on this would > > result in the wrong value potentially. Consider for example: > > > > 1. Enable pipe A + B > > 2. sagv_reject would be calculated as 0x3 > > 3. Disable pipe B > > 4. sagv_reject will have pipe B removed, leaving its value at 0x1 > > 5. No SAGV even though we only have one pipe enabled, which is wrong > > Quite good example. I think it means that pipe_sagv_mask should > be affected not only by per crtc checks, but also by overall active_pipes > state then. I.e ok, I remove active_pipes from intel_crtc_can_enable_sagv, > however that active_pipes && !is_power_of_2(active_pipes) check should > then also assign pipe_sagv_reject mask to ~0, if it evaluates to true. That would clobber the bits of sagv_reject for all crtcs, not just the ones part of the state currently. Ie. the very same example I gave would still do the wrong thing. > > I.e then we will always have intel_can_enable_sagv result reflected in > sagv mask, which is handy, also intel_can_enable_sagv would still > just evaluate sagv mask. > > Stan > > > > > > I think active_pipes should always affect the SAGV mask > > > otherwise we do get really strange situation: you have SAGV mask as 0, > > > but you still reject SAGV. So there is no way even then to track what > > > was the previous SAGV state - even if it's 0 it could have been rejected. > > > > > > IMO that is quite weird side effect. So removing active_pipes from > > > intel_crtc_can_enable_sagv doesn't sound like good idea. > > > > > > I think it is now just a bit too much hassle around simple > > > active_pipes_calculated boolean check. > > > > > > Stan > > > > > > > > > > > > > > > > > In fact that is the only reason I need active pipes here - otherwise I think > > > > > it was even your comment that we actually don't need those here at all, > > > > > as we just iterate through crtcs in state - pretty clearly remember we discussed > > > > > this. Just same way how it's done in intel bw check and other places. > > > > > > > > > > Stan > > > > > > > > > > > > > > > > > compute_sagv() { > > > > > > for_each_crtc() { > > > > > > if (crtc_can_sagv()) > > > > > > sagv_reject &= ~pipe; > > > > > > else > > > > > > sagv_reject |= pipe; > > > > > > } > > > > > > > > > > > > active_pipes = calc_active_pipes(); > > > > > > > > > > > > ... lock/serialize etc. > > > > > > } > > > > > > > > > > > > That way we don't have to update sagv_reject at all based on > > > > > > active_pipes. I think that even makes more sense since the > > > > > > active_pipes check is a global thing and not tied to any specific > > > > > > crtc. > > > > > > > > > > > > We can then make the check conditional on pre-icl (or whatever we want) > > > > > > in a later patch. And finally we can remove it altogether in a separate > > > > > > patch, since I don't think we should have to do it on any platform. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > We're also going to need to lock_global_state() if bw_state->active_pipes > > > > > > > > mask changes. > > > > > > > > > > > > > > Ohh.. right. > > > > > > > > > > > > > > > > > > > > > Stan > > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > > if (intel_crtc_can_enable_sagv(new_crtc_state)) > > > > > > > > > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); > > > > > > > > > else > > > > > > > > > @@ -5911,11 +5918,9 @@ skl_compute_wm(struct intel_atomic_state *state) > > > > > > > > > if (ret) > > > > > > > > > return ret; > > > > > > > > > > > > > > > > > > - if (state->modeset) { > > > > > > > > > - ret = intel_compute_sagv_mask(state); > > > > > > > > > - if (ret) > > > > > > > > > - return ret; > > > > > > > > > - } > > > > > > > > > + ret = intel_compute_sagv_mask(state); > > > > > > > > > + if (ret) > > > > > > > > > + return ret; > > > > > > > > > > > > > > > > We also need to remove the state->modeset checks around > > > > > > > > sagv_{pre,post}_update(). > > > > > > > > > > > > > > > > > > > > > > > > > > /* > > > > > > > > > * skl_compute_ddb() will have adjusted the final watermarks > > > > > > > > > -- > > > > > > > > > 2.24.1.485.gad05a3d8e5 > > > > > > > > > > > > > > > > -- > > > > > > > > Ville Syrjälä > > > > > > > > Intel > > > > > > > > > > > > -- > > > > > > Ville Syrjälä > > > > > > Intel > > > > > > > > -- > > > > Ville Syrjälä > > > > Intel > > > > -- > > Ville Syrjälä > > Intel
On Thu, Apr 30, 2020 at 02:40:37PM +0300, Ville Syrjälä wrote: > On Thu, Apr 30, 2020 at 02:29:51PM +0300, Lisovskiy, Stanislav wrote: > > On Thu, Apr 30, 2020 at 02:22:02PM +0300, Ville Syrjälä wrote: > > > On Thu, Apr 30, 2020 at 02:07:02PM +0300, Lisovskiy, Stanislav wrote: > > > > On Thu, Apr 30, 2020 at 01:55:59PM +0300, Ville Syrjälä wrote: > > > > > On Thu, Apr 30, 2020 at 01:47:02PM +0300, Lisovskiy, Stanislav wrote: > > > > > > On Thu, Apr 30, 2020 at 01:32:17PM +0300, Ville Syrjälä wrote: > > > > > > > On Thu, Apr 30, 2020 at 01:05:15PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > On Thu, Apr 30, 2020 at 12:21:04PM +0300, Ville Syrjälä wrote: > > > > > > > > > On Thu, Apr 23, 2020 at 10:58:56AM +0300, Stanislav Lisovskiy wrote: > > > > > > > > > > We need to calculate SAGV mask also in a non-modeset > > > > > > > > > > commit, however currently active_pipes are only calculated > > > > > > > > > > for modesets in global atomic state, thus now we will be > > > > > > > > > > tracking those also in bw_state in order to be able to > > > > > > > > > > properly access global data. > > > > > > > > > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > > > > --- > > > > > > > > > > drivers/gpu/drm/i915/display/intel_bw.h | 3 +++ > > > > > > > > > > drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++----- > > > > > > > > > > 2 files changed, 13 insertions(+), 5 deletions(-) > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h > > > > > > > > > > index d6df91058223..898b4a85ccab 100644 > > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_bw.h > > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_bw.h > > > > > > > > > > @@ -26,6 +26,9 @@ struct intel_bw_state { > > > > > > > > > > > > > > > > > > > > unsigned int data_rate[I915_MAX_PIPES]; > > > > > > > > > > u8 num_active_planes[I915_MAX_PIPES]; > > > > > > > > > > + > > > > > > > > > > + /* bitmask of active pipes */ > > > > > > > > > > + u8 active_pipes; > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > > > #define to_intel_bw_state(x) container_of((x), struct intel_bw_state, base) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > > > > > > > index 7e15cf3368ad..f7249bca3f6f 100644 > > > > > > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > > > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > > > > > > > @@ -3874,6 +3874,7 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > > > > > > struct intel_bw_state *new_bw_state = NULL; > > > > > > > > > > const struct intel_bw_state *old_bw_state = NULL; > > > > > > > > > > int i; > > > > > > > > > > + bool active_pipes_calculated = false; > > > > > > > > > > > > > > > > > > > > for_each_new_intel_crtc_in_state(state, crtc, > > > > > > > > > > new_crtc_state, i) { > > > > > > > > > > @@ -3883,6 +3884,12 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > > > > > > > > > > > > > > > > old_bw_state = intel_atomic_get_old_bw_state(state); > > > > > > > > > > > > > > > > > > > > + if (!active_pipes_calculated) { > > > > > > > > > > + state->active_pipes = new_bw_state->active_pipes = > > > > > > > > > > > > > > > > > > I don't think we should touch state->active_pipes here. > > > > > > > > > > > > > > > > Well, that was my question actually here as well. I understand that changing > > > > > > > > state->active_pipes here feels like some unneeded side effect, however having > > > > > > > > state->active_pipes and bw_state->active_pipes going out of sync doesn't sound > > > > > > > > very attractive to me either. That is why I don't like this idea of duplication > > > > > > > > at all - having constant need to sync those state, bw_state, cdclk_state, because > > > > > > > > they all might have different active_pipes now. > > > > > > > > > > > > > > Having an out of date active_pipes anywhere would be a bug in that > > > > > > > specific code. Also state->active_pipes is definitely going the way of > > > > > > > the dodo soon. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + intel_calc_active_pipes(state, old_bw_state->active_pipes); > > > > > > > > > > + active_pipes_calculated = true; > > > > > > > > > > + } > > > > > > > > > > > > > > > > > > I'd do this after the loop so we don't need this extra boolean. As far > > > > > > > > > as the active_pipes check in intel_crtc_can_enable_sagv(), I think we > > > > > > > > > can pull it out into intel_compute_sagv_mask() so that we do the check > > > > > > > > > after computing the mask. And of course change it to use > > > > > > > > > bw_state->active_pipes instead. > > > > > > > > > > > > > > > > intel_crtc_can_enable_sagv is called per crtc - so can't just pull it out, > > > > > > > > will have to have to cycles then - one will compute bw_state->active_pipes, > > > > > > > > and another pipe_sagv_mask. > > > > > > > > > > > > > > Hmm. Actually I think what we should probably do is keep the > > > > > > > active_pipes check in intel_can_enable_sagv(). Ie something like this: > > > > > > > > > > > > > > intel_can_enable_sagv(bw_state) { > > > > > > > if (active_pipes && !is_power_of_2(active_pipes)) > > > > > > > return false; > > > > > > > return sagv_reject != 0; > > > > > > > } > > > > > > > > > > > > I need active_pipes check here for skl code only, as it disables SAGV for multipipe > > > > > > scenarios. Adding this here would generalize it for other platforms and we > > > > > > don't want that for ICL+. > > > > > > > > > > Which is why I said "We can then make the check conditional on pre-icl > > > > > (or whatever we want) in a later patch". Why in a later patch? Because > > > > > currently the check is unconditional and it's generally a good idea to > > > > > limit the number of functional changes per patch to a minimum. > > > > > > > > Moving active_pipes check out of intel_crtc_can_enable_sagv will result > > > > in wrong SAGV mask calculated. > > > > > > > > i.e if you have 2 pipes, > > > > > > > > for_each_crtc() { > > > > if (crtc_can_sagv()) > > > > sagv_reject &= ~pipe; > > > > else > > > > sagv_reject |= pipe; > > > > } > > > > > > > > will calculate sagv_reject as 0 which is wrong and value will be stored > > > > in global state. > > > > > > No, it accurately reflects whether each of those pipes is capable of > > > sagv. The single pipe restriction is an additinal constrain on top of > > > that. In fact I think adjusting sagv_reject_mask based on this would > > > result in the wrong value potentially. Consider for example: > > > > > > 1. Enable pipe A + B > > > 2. sagv_reject would be calculated as 0x3 > > > 3. Disable pipe B > > > 4. sagv_reject will have pipe B removed, leaving its value at 0x1 > > > 5. No SAGV even though we only have one pipe enabled, which is wrong > > > > Quite good example. I think it means that pipe_sagv_mask should > > be affected not only by per crtc checks, but also by overall active_pipes > > state then. I.e ok, I remove active_pipes from intel_crtc_can_enable_sagv, > > however that active_pipes && !is_power_of_2(active_pipes) check should > > then also assign pipe_sagv_reject mask to ~0, if it evaluates to true. > > That would clobber the bits of sagv_reject for all crtcs, not just > the ones part of the state currently. Ie. the very same example > I gave would still do the wrong thing. Sounds like we might want to reserve one bit from SAGV mask as "global SAGV reject" flag, for non-crtc related checks. I.e if we have active_pipes > 1 for skl, we just set sagv_mask |= 0x80 for instance - that will make your example work and also we will be storing SAGV result in a single variable. Stan > > > > > I.e then we will always have intel_can_enable_sagv result reflected in > > sagv mask, which is handy, also intel_can_enable_sagv would still > > just evaluate sagv mask. > > > > Stan > > > > > > > > > I think active_pipes should always affect the SAGV mask > > > > otherwise we do get really strange situation: you have SAGV mask as 0, > > > > but you still reject SAGV. So there is no way even then to track what > > > > was the previous SAGV state - even if it's 0 it could have been rejected. > > > > > > > > IMO that is quite weird side effect. So removing active_pipes from > > > > intel_crtc_can_enable_sagv doesn't sound like good idea. > > > > > > > > I think it is now just a bit too much hassle around simple > > > > active_pipes_calculated boolean check. > > > > > > > > Stan > > > > > > > > > > > > > > > > > > > > > In fact that is the only reason I need active pipes here - otherwise I think > > > > > > it was even your comment that we actually don't need those here at all, > > > > > > as we just iterate through crtcs in state - pretty clearly remember we discussed > > > > > > this. Just same way how it's done in intel bw check and other places. > > > > > > > > > > > > Stan > > > > > > > > > > > > > > > > > > > > compute_sagv() { > > > > > > > for_each_crtc() { > > > > > > > if (crtc_can_sagv()) > > > > > > > sagv_reject &= ~pipe; > > > > > > > else > > > > > > > sagv_reject |= pipe; > > > > > > > } > > > > > > > > > > > > > > active_pipes = calc_active_pipes(); > > > > > > > > > > > > > > ... lock/serialize etc. > > > > > > > } > > > > > > > > > > > > > > That way we don't have to update sagv_reject at all based on > > > > > > > active_pipes. I think that even makes more sense since the > > > > > > > active_pipes check is a global thing and not tied to any specific > > > > > > > crtc. > > > > > > > > > > > > > > We can then make the check conditional on pre-icl (or whatever we want) > > > > > > > in a later patch. And finally we can remove it altogether in a separate > > > > > > > patch, since I don't think we should have to do it on any platform. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > We're also going to need to lock_global_state() if bw_state->active_pipes > > > > > > > > > mask changes. > > > > > > > > > > > > > > > > Ohh.. right. > > > > > > > > > > > > > > > > > > > > > > > > Stan > > > > > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > > > if (intel_crtc_can_enable_sagv(new_crtc_state)) > > > > > > > > > > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); > > > > > > > > > > else > > > > > > > > > > @@ -5911,11 +5918,9 @@ skl_compute_wm(struct intel_atomic_state *state) > > > > > > > > > > if (ret) > > > > > > > > > > return ret; > > > > > > > > > > > > > > > > > > > > - if (state->modeset) { > > > > > > > > > > - ret = intel_compute_sagv_mask(state); > > > > > > > > > > - if (ret) > > > > > > > > > > - return ret; > > > > > > > > > > - } > > > > > > > > > > + ret = intel_compute_sagv_mask(state); > > > > > > > > > > + if (ret) > > > > > > > > > > + return ret; > > > > > > > > > > > > > > > > > > We also need to remove the state->modeset checks around > > > > > > > > > sagv_{pre,post}_update(). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > /* > > > > > > > > > > * skl_compute_ddb() will have adjusted the final watermarks > > > > > > > > > > -- > > > > > > > > > > 2.24.1.485.gad05a3d8e5 > > > > > > > > > > > > > > > > > > -- > > > > > > > > > Ville Syrjälä > > > > > > > > > Intel > > > > > > > > > > > > > > -- > > > > > > > Ville Syrjälä > > > > > > > Intel > > > > > > > > > > -- > > > > > Ville Syrjälä > > > > > Intel > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel
diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h index d6df91058223..898b4a85ccab 100644 --- a/drivers/gpu/drm/i915/display/intel_bw.h +++ b/drivers/gpu/drm/i915/display/intel_bw.h @@ -26,6 +26,9 @@ struct intel_bw_state { unsigned int data_rate[I915_MAX_PIPES]; u8 num_active_planes[I915_MAX_PIPES]; + + /* bitmask of active pipes */ + u8 active_pipes; }; #define to_intel_bw_state(x) container_of((x), struct intel_bw_state, base) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 7e15cf3368ad..f7249bca3f6f 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3874,6 +3874,7 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) struct intel_bw_state *new_bw_state = NULL; const struct intel_bw_state *old_bw_state = NULL; int i; + bool active_pipes_calculated = false; for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) { @@ -3883,6 +3884,12 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) old_bw_state = intel_atomic_get_old_bw_state(state); + if (!active_pipes_calculated) { + state->active_pipes = new_bw_state->active_pipes = + intel_calc_active_pipes(state, old_bw_state->active_pipes); + active_pipes_calculated = true; + } + if (intel_crtc_can_enable_sagv(new_crtc_state)) new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); else @@ -5911,11 +5918,9 @@ skl_compute_wm(struct intel_atomic_state *state) if (ret) return ret; - if (state->modeset) { - ret = intel_compute_sagv_mask(state); - if (ret) - return ret; - } + ret = intel_compute_sagv_mask(state); + if (ret) + return ret; /* * skl_compute_ddb() will have adjusted the final watermarks
We need to calculate SAGV mask also in a non-modeset commit, however currently active_pipes are only calculated for modesets in global atomic state, thus now we will be tracking those also in bw_state in order to be able to properly access global data. Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> --- drivers/gpu/drm/i915/display/intel_bw.h | 3 +++ drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++----- 2 files changed, 13 insertions(+), 5 deletions(-)