| Message ID | 20170501133434.5683-5-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 01, 2017 at 03:34:34PM +0200, Maarten Lankhorst wrote: > The watermarks it should calculate against are the old optimal watermarks. > The currently active crtc watermarks are pure fiction, and are invalid in > case of a nonblocking modeset, page flip enabling/disabling planes or any > other reason. > > When the crtc is disabled or during a modeset the intermediate watermarks > don't need to be programmed separately, and could be directly assigned > to the optimal watermarks. > > Also rename crtc_state to new_crtc_state, to distinguish it from the old state. > > Changes since v1: > - Use intel_atomic_get_old_crtc_state. (ville) > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++------ > 1 file changed, 14 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 0f344b1fff45..a09396ee1f3d 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -1458,16 +1458,24 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state, > > static int vlv_compute_intermediate_wm(struct drm_device *dev, > struct intel_crtc *crtc, > - struct intel_crtc_state *crtc_state) > + struct intel_crtc_state *new_crtc_state) > { > - struct vlv_wm_state *intermediate = &crtc_state->wm.vlv.intermediate; > - const struct vlv_wm_state *optimal = &crtc_state->wm.vlv.optimal; > - const struct vlv_wm_state *active = &crtc->wm.active.vlv; > + struct vlv_wm_state *intermediate = &new_crtc_state->wm.vlv.intermediate; > + const struct vlv_wm_state *optimal = &new_crtc_state->wm.vlv.optimal; > + const struct intel_crtc_state *old_crtc_state = > + intel_atomic_get_old_crtc_state(new_crtc_state->base.state, crtc); > + const struct vlv_wm_state *active = &old_crtc_state->wm.vlv.optimal; > int level; > > + if (!new_crtc_state->base.active || drm_atomic_crtc_needs_modeset(&new_crtc_state->base)) { > + *intermediate = *optimal; > + > + return 0; > + } > + > intermediate->num_levels = min(optimal->num_levels, active->num_levels); > intermediate->cxsr = optimal->cxsr && active->cxsr && > - !crtc_state->disable_cxsr; > + !new_crtc_state->disable_cxsr; We need to consider disable_cxsr even in the modeset case. > > for (level = 0; level < intermediate->num_levels; level++) { > enum plane_id plane_id; > @@ -1491,7 +1499,7 @@ static int vlv_compute_intermediate_wm(struct drm_device *dev, > * omit the post-vblank programming; only update if it's different. > */ > if (memcmp(intermediate, optimal, sizeof(*intermediate)) != 0) > - crtc_state->wm.need_postvbl_update = true; > + new_crtc_state->wm.need_postvbl_update = true; > > return 0; > } > -- > 2.9.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 03-05-17 om 15:45 schreef Ville Syrjälä: > On Mon, May 01, 2017 at 03:34:34PM +0200, Maarten Lankhorst wrote: >> The watermarks it should calculate against are the old optimal watermarks. >> The currently active crtc watermarks are pure fiction, and are invalid in >> case of a nonblocking modeset, page flip enabling/disabling planes or any >> other reason. >> >> When the crtc is disabled or during a modeset the intermediate watermarks >> don't need to be programmed separately, and could be directly assigned >> to the optimal watermarks. >> >> Also rename crtc_state to new_crtc_state, to distinguish it from the old state. >> >> Changes since v1: >> - Use intel_atomic_get_old_crtc_state. (ville) >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++------ >> 1 file changed, 14 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >> index 0f344b1fff45..a09396ee1f3d 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -1458,16 +1458,24 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state, >> >> static int vlv_compute_intermediate_wm(struct drm_device *dev, >> struct intel_crtc *crtc, >> - struct intel_crtc_state *crtc_state) >> + struct intel_crtc_state *new_crtc_state) >> { >> - struct vlv_wm_state *intermediate = &crtc_state->wm.vlv.intermediate; >> - const struct vlv_wm_state *optimal = &crtc_state->wm.vlv.optimal; >> - const struct vlv_wm_state *active = &crtc->wm.active.vlv; >> + struct vlv_wm_state *intermediate = &new_crtc_state->wm.vlv.intermediate; >> + const struct vlv_wm_state *optimal = &new_crtc_state->wm.vlv.optimal; >> + const struct intel_crtc_state *old_crtc_state = >> + intel_atomic_get_old_crtc_state(new_crtc_state->base.state, crtc); >> + const struct vlv_wm_state *active = &old_crtc_state->wm.vlv.optimal; >> int level; >> >> + if (!new_crtc_state->base.active || drm_atomic_crtc_needs_modeset(&new_crtc_state->base)) { >> + *intermediate = *optimal; >> + >> + return 0; >> + } >> + >> intermediate->num_levels = min(optimal->num_levels, active->num_levels); >> intermediate->cxsr = optimal->cxsr && active->cxsr && >> - !crtc_state->disable_cxsr; >> + !new_crtc_state->disable_cxsr; > We need to consider disable_cxsr even in the modeset case. Why is this? crtc_state->disable_cxsr is set if any plane is part of the crtc during modeset, so it's disabled during modeset already. ~Maarten
On Wed, May 03, 2017 at 04:06:37PM +0200, Maarten Lankhorst wrote: > Op 03-05-17 om 15:45 schreef Ville Syrjälä: > > On Mon, May 01, 2017 at 03:34:34PM +0200, Maarten Lankhorst wrote: > >> The watermarks it should calculate against are the old optimal watermarks. > >> The currently active crtc watermarks are pure fiction, and are invalid in > >> case of a nonblocking modeset, page flip enabling/disabling planes or any > >> other reason. > >> > >> When the crtc is disabled or during a modeset the intermediate watermarks > >> don't need to be programmed separately, and could be directly assigned > >> to the optimal watermarks. > >> > >> Also rename crtc_state to new_crtc_state, to distinguish it from the old state. > >> > >> Changes since v1: > >> - Use intel_atomic_get_old_crtc_state. (ville) > >> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++------ > >> 1 file changed, 14 insertions(+), 6 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > >> index 0f344b1fff45..a09396ee1f3d 100644 > >> --- a/drivers/gpu/drm/i915/intel_pm.c > >> +++ b/drivers/gpu/drm/i915/intel_pm.c > >> @@ -1458,16 +1458,24 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state, > >> > >> static int vlv_compute_intermediate_wm(struct drm_device *dev, > >> struct intel_crtc *crtc, > >> - struct intel_crtc_state *crtc_state) > >> + struct intel_crtc_state *new_crtc_state) > >> { > >> - struct vlv_wm_state *intermediate = &crtc_state->wm.vlv.intermediate; > >> - const struct vlv_wm_state *optimal = &crtc_state->wm.vlv.optimal; > >> - const struct vlv_wm_state *active = &crtc->wm.active.vlv; > >> + struct vlv_wm_state *intermediate = &new_crtc_state->wm.vlv.intermediate; > >> + const struct vlv_wm_state *optimal = &new_crtc_state->wm.vlv.optimal; > >> + const struct intel_crtc_state *old_crtc_state = > >> + intel_atomic_get_old_crtc_state(new_crtc_state->base.state, crtc); > >> + const struct vlv_wm_state *active = &old_crtc_state->wm.vlv.optimal; > >> int level; > >> > >> + if (!new_crtc_state->base.active || drm_atomic_crtc_needs_modeset(&new_crtc_state->base)) { > >> + *intermediate = *optimal; > >> + > >> + return 0; > >> + } > >> + > >> intermediate->num_levels = min(optimal->num_levels, active->num_levels); > >> intermediate->cxsr = optimal->cxsr && active->cxsr && > >> - !crtc_state->disable_cxsr; > >> + !new_crtc_state->disable_cxsr; > > We need to consider disable_cxsr even in the modeset case. > Why is this? crtc_state->disable_cxsr is set if any plane is part of the crtc during modeset, so it's disabled during modeset already. It's set if any plane is enabling/disabling, which should be quite typical during a modeset.
Op 03-05-17 om 16:11 schreef Ville Syrjälä: > On Wed, May 03, 2017 at 04:06:37PM +0200, Maarten Lankhorst wrote: >> Op 03-05-17 om 15:45 schreef Ville Syrjälä: >>> On Mon, May 01, 2017 at 03:34:34PM +0200, Maarten Lankhorst wrote: >>>> The watermarks it should calculate against are the old optimal watermarks. >>>> The currently active crtc watermarks are pure fiction, and are invalid in >>>> case of a nonblocking modeset, page flip enabling/disabling planes or any >>>> other reason. >>>> >>>> When the crtc is disabled or during a modeset the intermediate watermarks >>>> don't need to be programmed separately, and could be directly assigned >>>> to the optimal watermarks. >>>> >>>> Also rename crtc_state to new_crtc_state, to distinguish it from the old state. >>>> >>>> Changes since v1: >>>> - Use intel_atomic_get_old_crtc_state. (ville) >>>> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++------ >>>> 1 file changed, 14 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >>>> index 0f344b1fff45..a09396ee1f3d 100644 >>>> --- a/drivers/gpu/drm/i915/intel_pm.c >>>> +++ b/drivers/gpu/drm/i915/intel_pm.c >>>> @@ -1458,16 +1458,24 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state, >>>> >>>> static int vlv_compute_intermediate_wm(struct drm_device *dev, >>>> struct intel_crtc *crtc, >>>> - struct intel_crtc_state *crtc_state) >>>> + struct intel_crtc_state *new_crtc_state) >>>> { >>>> - struct vlv_wm_state *intermediate = &crtc_state->wm.vlv.intermediate; >>>> - const struct vlv_wm_state *optimal = &crtc_state->wm.vlv.optimal; >>>> - const struct vlv_wm_state *active = &crtc->wm.active.vlv; >>>> + struct vlv_wm_state *intermediate = &new_crtc_state->wm.vlv.intermediate; >>>> + const struct vlv_wm_state *optimal = &new_crtc_state->wm.vlv.optimal; >>>> + const struct intel_crtc_state *old_crtc_state = >>>> + intel_atomic_get_old_crtc_state(new_crtc_state->base.state, crtc); >>>> + const struct vlv_wm_state *active = &old_crtc_state->wm.vlv.optimal; >>>> int level; >>>> >>>> + if (!new_crtc_state->base.active || drm_atomic_crtc_needs_modeset(&new_crtc_state->base)) { >>>> + *intermediate = *optimal; >>>> + >>>> + return 0; >>>> + } >>>> + >>>> intermediate->num_levels = min(optimal->num_levels, active->num_levels); >>>> intermediate->cxsr = optimal->cxsr && active->cxsr && >>>> - !crtc_state->disable_cxsr; >>>> + !new_crtc_state->disable_cxsr; >>> We need to consider disable_cxsr even in the modeset case. >> Why is this? crtc_state->disable_cxsr is set if any plane is part of the crtc during modeset, so it's disabled during modeset already. > It's set if any plane is enabling/disabling, which should be quite > typical during a modeset. Yeah but .initial_watermarks is called during crtc_enable, so cxsr will get enabled anyway. Maybe I'm missing something, but why are the intermediate watermarks and optimize_watermarks needed here? ~Maarten
On Wed, May 03, 2017 at 05:53:34PM +0200, Maarten Lankhorst wrote: > Op 03-05-17 om 16:11 schreef Ville Syrjälä: > > On Wed, May 03, 2017 at 04:06:37PM +0200, Maarten Lankhorst wrote: > >> Op 03-05-17 om 15:45 schreef Ville Syrjälä: > >>> On Mon, May 01, 2017 at 03:34:34PM +0200, Maarten Lankhorst wrote: > >>>> The watermarks it should calculate against are the old optimal watermarks. > >>>> The currently active crtc watermarks are pure fiction, and are invalid in > >>>> case of a nonblocking modeset, page flip enabling/disabling planes or any > >>>> other reason. > >>>> > >>>> When the crtc is disabled or during a modeset the intermediate watermarks > >>>> don't need to be programmed separately, and could be directly assigned > >>>> to the optimal watermarks. > >>>> > >>>> Also rename crtc_state to new_crtc_state, to distinguish it from the old state. > >>>> > >>>> Changes since v1: > >>>> - Use intel_atomic_get_old_crtc_state. (ville) > >>>> > >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>>> --- > >>>> drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++------ > >>>> 1 file changed, 14 insertions(+), 6 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > >>>> index 0f344b1fff45..a09396ee1f3d 100644 > >>>> --- a/drivers/gpu/drm/i915/intel_pm.c > >>>> +++ b/drivers/gpu/drm/i915/intel_pm.c > >>>> @@ -1458,16 +1458,24 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state, > >>>> > >>>> static int vlv_compute_intermediate_wm(struct drm_device *dev, > >>>> struct intel_crtc *crtc, > >>>> - struct intel_crtc_state *crtc_state) > >>>> + struct intel_crtc_state *new_crtc_state) > >>>> { > >>>> - struct vlv_wm_state *intermediate = &crtc_state->wm.vlv.intermediate; > >>>> - const struct vlv_wm_state *optimal = &crtc_state->wm.vlv.optimal; > >>>> - const struct vlv_wm_state *active = &crtc->wm.active.vlv; > >>>> + struct vlv_wm_state *intermediate = &new_crtc_state->wm.vlv.intermediate; > >>>> + const struct vlv_wm_state *optimal = &new_crtc_state->wm.vlv.optimal; > >>>> + const struct intel_crtc_state *old_crtc_state = > >>>> + intel_atomic_get_old_crtc_state(new_crtc_state->base.state, crtc); > >>>> + const struct vlv_wm_state *active = &old_crtc_state->wm.vlv.optimal; > >>>> int level; > >>>> > >>>> + if (!new_crtc_state->base.active || drm_atomic_crtc_needs_modeset(&new_crtc_state->base)) { > >>>> + *intermediate = *optimal; > >>>> + > >>>> + return 0; > >>>> + } > >>>> + > >>>> intermediate->num_levels = min(optimal->num_levels, active->num_levels); > >>>> intermediate->cxsr = optimal->cxsr && active->cxsr && > >>>> - !crtc_state->disable_cxsr; > >>>> + !new_crtc_state->disable_cxsr; > >>> We need to consider disable_cxsr even in the modeset case. > >> Why is this? crtc_state->disable_cxsr is set if any plane is part of the crtc during modeset, so it's disabled during modeset already. > > It's set if any plane is enabling/disabling, which should be quite > > typical during a modeset. > > Yeah but .initial_watermarks is called during crtc_enable, so cxsr will get enabled anyway. Which is not what we want. CxSR must stay off until the planes have been enabled.
Op 03-05-17 om 18:07 schreef Ville Syrjälä: > On Wed, May 03, 2017 at 05:53:34PM +0200, Maarten Lankhorst wrote: >> Op 03-05-17 om 16:11 schreef Ville Syrjälä: >>> On Wed, May 03, 2017 at 04:06:37PM +0200, Maarten Lankhorst wrote: >>>> Op 03-05-17 om 15:45 schreef Ville Syrjälä: >>>>> On Mon, May 01, 2017 at 03:34:34PM +0200, Maarten Lankhorst wrote: >>>>>> The watermarks it should calculate against are the old optimal watermarks. >>>>>> The currently active crtc watermarks are pure fiction, and are invalid in >>>>>> case of a nonblocking modeset, page flip enabling/disabling planes or any >>>>>> other reason. >>>>>> >>>>>> When the crtc is disabled or during a modeset the intermediate watermarks >>>>>> don't need to be programmed separately, and could be directly assigned >>>>>> to the optimal watermarks. >>>>>> >>>>>> Also rename crtc_state to new_crtc_state, to distinguish it from the old state. >>>>>> >>>>>> Changes since v1: >>>>>> - Use intel_atomic_get_old_crtc_state. (ville) >>>>>> >>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>>>> --- >>>>>> drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++------ >>>>>> 1 file changed, 14 insertions(+), 6 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >>>>>> index 0f344b1fff45..a09396ee1f3d 100644 >>>>>> --- a/drivers/gpu/drm/i915/intel_pm.c >>>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c >>>>>> @@ -1458,16 +1458,24 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state, >>>>>> >>>>>> static int vlv_compute_intermediate_wm(struct drm_device *dev, >>>>>> struct intel_crtc *crtc, >>>>>> - struct intel_crtc_state *crtc_state) >>>>>> + struct intel_crtc_state *new_crtc_state) >>>>>> { >>>>>> - struct vlv_wm_state *intermediate = &crtc_state->wm.vlv.intermediate; >>>>>> - const struct vlv_wm_state *optimal = &crtc_state->wm.vlv.optimal; >>>>>> - const struct vlv_wm_state *active = &crtc->wm.active.vlv; >>>>>> + struct vlv_wm_state *intermediate = &new_crtc_state->wm.vlv.intermediate; >>>>>> + const struct vlv_wm_state *optimal = &new_crtc_state->wm.vlv.optimal; >>>>>> + const struct intel_crtc_state *old_crtc_state = >>>>>> + intel_atomic_get_old_crtc_state(new_crtc_state->base.state, crtc); >>>>>> + const struct vlv_wm_state *active = &old_crtc_state->wm.vlv.optimal; >>>>>> int level; >>>>>> >>>>>> + if (!new_crtc_state->base.active || drm_atomic_crtc_needs_modeset(&new_crtc_state->base)) { >>>>>> + *intermediate = *optimal; >>>>>> + >>>>>> + return 0; >>>>>> + } >>>>>> + >>>>>> intermediate->num_levels = min(optimal->num_levels, active->num_levels); >>>>>> intermediate->cxsr = optimal->cxsr && active->cxsr && >>>>>> - !crtc_state->disable_cxsr; >>>>>> + !new_crtc_state->disable_cxsr; >>>>> We need to consider disable_cxsr even in the modeset case. >>>> Why is this? crtc_state->disable_cxsr is set if any plane is part of the crtc during modeset, so it's disabled during modeset already. >>> It's set if any plane is enabling/disabling, which should be quite >>> typical during a modeset. >> Yeah but .initial_watermarks is called during crtc_enable, so cxsr will get enabled anyway. > Which is not what we want. CxSR must stay off until the planes have been > enabled. > In that case why is it enabled in .initial_watermarks at all? It should be in optimize_watermarks then..
On Wed, May 03, 2017 at 06:18:46PM +0200, Maarten Lankhorst wrote: > Op 03-05-17 om 18:07 schreef Ville Syrjälä: > > On Wed, May 03, 2017 at 05:53:34PM +0200, Maarten Lankhorst wrote: > >> Op 03-05-17 om 16:11 schreef Ville Syrjälä: > >>> On Wed, May 03, 2017 at 04:06:37PM +0200, Maarten Lankhorst wrote: > >>>> Op 03-05-17 om 15:45 schreef Ville Syrjälä: > >>>>> On Mon, May 01, 2017 at 03:34:34PM +0200, Maarten Lankhorst wrote: > >>>>>> The watermarks it should calculate against are the old optimal watermarks. > >>>>>> The currently active crtc watermarks are pure fiction, and are invalid in > >>>>>> case of a nonblocking modeset, page flip enabling/disabling planes or any > >>>>>> other reason. > >>>>>> > >>>>>> When the crtc is disabled or during a modeset the intermediate watermarks > >>>>>> don't need to be programmed separately, and could be directly assigned > >>>>>> to the optimal watermarks. > >>>>>> > >>>>>> Also rename crtc_state to new_crtc_state, to distinguish it from the old state. > >>>>>> > >>>>>> Changes since v1: > >>>>>> - Use intel_atomic_get_old_crtc_state. (ville) > >>>>>> > >>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>>>>> --- > >>>>>> drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++------ > >>>>>> 1 file changed, 14 insertions(+), 6 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > >>>>>> index 0f344b1fff45..a09396ee1f3d 100644 > >>>>>> --- a/drivers/gpu/drm/i915/intel_pm.c > >>>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c > >>>>>> @@ -1458,16 +1458,24 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state, > >>>>>> > >>>>>> static int vlv_compute_intermediate_wm(struct drm_device *dev, > >>>>>> struct intel_crtc *crtc, > >>>>>> - struct intel_crtc_state *crtc_state) > >>>>>> + struct intel_crtc_state *new_crtc_state) > >>>>>> { > >>>>>> - struct vlv_wm_state *intermediate = &crtc_state->wm.vlv.intermediate; > >>>>>> - const struct vlv_wm_state *optimal = &crtc_state->wm.vlv.optimal; > >>>>>> - const struct vlv_wm_state *active = &crtc->wm.active.vlv; > >>>>>> + struct vlv_wm_state *intermediate = &new_crtc_state->wm.vlv.intermediate; > >>>>>> + const struct vlv_wm_state *optimal = &new_crtc_state->wm.vlv.optimal; > >>>>>> + const struct intel_crtc_state *old_crtc_state = > >>>>>> + intel_atomic_get_old_crtc_state(new_crtc_state->base.state, crtc); > >>>>>> + const struct vlv_wm_state *active = &old_crtc_state->wm.vlv.optimal; > >>>>>> int level; > >>>>>> > >>>>>> + if (!new_crtc_state->base.active || drm_atomic_crtc_needs_modeset(&new_crtc_state->base)) { > >>>>>> + *intermediate = *optimal; > >>>>>> + > >>>>>> + return 0; > >>>>>> + } > >>>>>> + > >>>>>> intermediate->num_levels = min(optimal->num_levels, active->num_levels); > >>>>>> intermediate->cxsr = optimal->cxsr && active->cxsr && > >>>>>> - !crtc_state->disable_cxsr; > >>>>>> + !new_crtc_state->disable_cxsr; > >>>>> We need to consider disable_cxsr even in the modeset case. > >>>> Why is this? crtc_state->disable_cxsr is set if any plane is part of the crtc during modeset, so it's disabled during modeset already. > >>> It's set if any plane is enabling/disabling, which should be quite > >>> typical during a modeset. > >> Yeah but .initial_watermarks is called during crtc_enable, so cxsr will get enabled anyway. > > Which is not what we want. CxSR must stay off until the planes have been > > enabled. > > > In that case why is it enabled in .initial_watermarks at all? It should be in optimize_watermarks then.. Because we can keep it enabled across the update unless planes are getting enabled or disabled.
Op 03-05-17 om 20:03 schreef Ville Syrjälä: > On Wed, May 03, 2017 at 06:18:46PM +0200, Maarten Lankhorst wrote: >> Op 03-05-17 om 18:07 schreef Ville Syrjälä: >>> On Wed, May 03, 2017 at 05:53:34PM +0200, Maarten Lankhorst wrote: >>>> Op 03-05-17 om 16:11 schreef Ville Syrjälä: >>>>> On Wed, May 03, 2017 at 04:06:37PM +0200, Maarten Lankhorst wrote: >>>>>> Op 03-05-17 om 15:45 schreef Ville Syrjälä: >>>>>>> On Mon, May 01, 2017 at 03:34:34PM +0200, Maarten Lankhorst wrote: >>>>>>>> The watermarks it should calculate against are the old optimal watermarks. >>>>>>>> The currently active crtc watermarks are pure fiction, and are invalid in >>>>>>>> case of a nonblocking modeset, page flip enabling/disabling planes or any >>>>>>>> other reason. >>>>>>>> >>>>>>>> When the crtc is disabled or during a modeset the intermediate watermarks >>>>>>>> don't need to be programmed separately, and could be directly assigned >>>>>>>> to the optimal watermarks. >>>>>>>> >>>>>>>> Also rename crtc_state to new_crtc_state, to distinguish it from the old state. >>>>>>>> >>>>>>>> Changes since v1: >>>>>>>> - Use intel_atomic_get_old_crtc_state. (ville) >>>>>>>> >>>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>>>>>> --- >>>>>>>> drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++------ >>>>>>>> 1 file changed, 14 insertions(+), 6 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >>>>>>>> index 0f344b1fff45..a09396ee1f3d 100644 >>>>>>>> --- a/drivers/gpu/drm/i915/intel_pm.c >>>>>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c >>>>>>>> @@ -1458,16 +1458,24 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state, >>>>>>>> >>>>>>>> static int vlv_compute_intermediate_wm(struct drm_device *dev, >>>>>>>> struct intel_crtc *crtc, >>>>>>>> - struct intel_crtc_state *crtc_state) >>>>>>>> + struct intel_crtc_state *new_crtc_state) >>>>>>>> { >>>>>>>> - struct vlv_wm_state *intermediate = &crtc_state->wm.vlv.intermediate; >>>>>>>> - const struct vlv_wm_state *optimal = &crtc_state->wm.vlv.optimal; >>>>>>>> - const struct vlv_wm_state *active = &crtc->wm.active.vlv; >>>>>>>> + struct vlv_wm_state *intermediate = &new_crtc_state->wm.vlv.intermediate; >>>>>>>> + const struct vlv_wm_state *optimal = &new_crtc_state->wm.vlv.optimal; >>>>>>>> + const struct intel_crtc_state *old_crtc_state = >>>>>>>> + intel_atomic_get_old_crtc_state(new_crtc_state->base.state, crtc); >>>>>>>> + const struct vlv_wm_state *active = &old_crtc_state->wm.vlv.optimal; >>>>>>>> int level; >>>>>>>> >>>>>>>> + if (!new_crtc_state->base.active || drm_atomic_crtc_needs_modeset(&new_crtc_state->base)) { >>>>>>>> + *intermediate = *optimal; >>>>>>>> + >>>>>>>> + return 0; >>>>>>>> + } >>>>>>>> + >>>>>>>> intermediate->num_levels = min(optimal->num_levels, active->num_levels); >>>>>>>> intermediate->cxsr = optimal->cxsr && active->cxsr && >>>>>>>> - !crtc_state->disable_cxsr; >>>>>>>> + !new_crtc_state->disable_cxsr; >>>>>>> We need to consider disable_cxsr even in the modeset case. >>>>>> Why is this? crtc_state->disable_cxsr is set if any plane is part of the crtc during modeset, so it's disabled during modeset already. >>>>> It's set if any plane is enabling/disabling, which should be quite >>>>> typical during a modeset. >>>> Yeah but .initial_watermarks is called during crtc_enable, so cxsr will get enabled anyway. >>> Which is not what we want. CxSR must stay off until the planes have been >>> enabled. >>> >> In that case why is it enabled in .initial_watermarks at all? It should be in optimize_watermarks then.. > Because we can keep it enabled across the update unless planes are > getting enabled or disabled. In that case the watermarks still need some fixing to handle this correctly, and optimize watermarks should be called on modeset too.
Op 03-05-17 om 20:03 schreef Ville Syrjälä: > On Wed, May 03, 2017 at 06:18:46PM +0200, Maarten Lankhorst wrote: >> Op 03-05-17 om 18:07 schreef Ville Syrjälä: >>> On Wed, May 03, 2017 at 05:53:34PM +0200, Maarten Lankhorst wrote: >>>> Op 03-05-17 om 16:11 schreef Ville Syrjälä: >>>>> On Wed, May 03, 2017 at 04:06:37PM +0200, Maarten Lankhorst wrote: >>>>>> Op 03-05-17 om 15:45 schreef Ville Syrjälä: >>>>>>> On Mon, May 01, 2017 at 03:34:34PM +0200, Maarten Lankhorst wrote: >>>>>>>> The watermarks it should calculate against are the old optimal watermarks. >>>>>>>> The currently active crtc watermarks are pure fiction, and are invalid in >>>>>>>> case of a nonblocking modeset, page flip enabling/disabling planes or any >>>>>>>> other reason. >>>>>>>> >>>>>>>> When the crtc is disabled or during a modeset the intermediate watermarks >>>>>>>> don't need to be programmed separately, and could be directly assigned >>>>>>>> to the optimal watermarks. >>>>>>>> >>>>>>>> Also rename crtc_state to new_crtc_state, to distinguish it from the old state. >>>>>>>> >>>>>>>> Changes since v1: >>>>>>>> - Use intel_atomic_get_old_crtc_state. (ville) >>>>>>>> >>>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>>>>>> --- >>>>>>>> drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++------ >>>>>>>> 1 file changed, 14 insertions(+), 6 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >>>>>>>> index 0f344b1fff45..a09396ee1f3d 100644 >>>>>>>> --- a/drivers/gpu/drm/i915/intel_pm.c >>>>>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c >>>>>>>> @@ -1458,16 +1458,24 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state, >>>>>>>> >>>>>>>> static int vlv_compute_intermediate_wm(struct drm_device *dev, >>>>>>>> struct intel_crtc *crtc, >>>>>>>> - struct intel_crtc_state *crtc_state) >>>>>>>> + struct intel_crtc_state *new_crtc_state) >>>>>>>> { >>>>>>>> - struct vlv_wm_state *intermediate = &crtc_state->wm.vlv.intermediate; >>>>>>>> - const struct vlv_wm_state *optimal = &crtc_state->wm.vlv.optimal; >>>>>>>> - const struct vlv_wm_state *active = &crtc->wm.active.vlv; >>>>>>>> + struct vlv_wm_state *intermediate = &new_crtc_state->wm.vlv.intermediate; >>>>>>>> + const struct vlv_wm_state *optimal = &new_crtc_state->wm.vlv.optimal; >>>>>>>> + const struct intel_crtc_state *old_crtc_state = >>>>>>>> + intel_atomic_get_old_crtc_state(new_crtc_state->base.state, crtc); >>>>>>>> + const struct vlv_wm_state *active = &old_crtc_state->wm.vlv.optimal; >>>>>>>> int level; >>>>>>>> >>>>>>>> + if (!new_crtc_state->base.active || drm_atomic_crtc_needs_modeset(&new_crtc_state->base)) { >>>>>>>> + *intermediate = *optimal; >>>>>>>> + >>>>>>>> + return 0; >>>>>>>> + } >>>>>>>> + >>>>>>>> intermediate->num_levels = min(optimal->num_levels, active->num_levels); >>>>>>>> intermediate->cxsr = optimal->cxsr && active->cxsr && >>>>>>>> - !crtc_state->disable_cxsr; >>>>>>>> + !new_crtc_state->disable_cxsr; >>>>>>> We need to consider disable_cxsr even in the modeset case. >>>>>> Why is this? crtc_state->disable_cxsr is set if any plane is part of the crtc during modeset, so it's disabled during modeset already. >>>>> It's set if any plane is enabling/disabling, which should be quite >>>>> typical during a modeset. >>>> Yeah but .initial_watermarks is called during crtc_enable, so cxsr will get enabled anyway. >>> Which is not what we want. CxSR must stay off until the planes have been >>> enabled. >>> >> In that case why is it enabled in .initial_watermarks at all? It should be in optimize_watermarks then.. > Because we can keep it enabled across the update unless planes are > getting enabled or disabled. > So for the modeset case, computing intermediate watermarks: *intermediate = *optimal; if (needs_modeset) intermediate->cxsr = false; if (optimal->cxsr && !intermediate->cxsr) new_crtc_state->wm.need_postvbl_update = true; ?
On Thu, May 04, 2017 at 10:12:52AM +0200, Maarten Lankhorst wrote: > Op 03-05-17 om 20:03 schreef Ville Syrjälä: > > On Wed, May 03, 2017 at 06:18:46PM +0200, Maarten Lankhorst wrote: > >> Op 03-05-17 om 18:07 schreef Ville Syrjälä: > >>> On Wed, May 03, 2017 at 05:53:34PM +0200, Maarten Lankhorst wrote: > >>>> Op 03-05-17 om 16:11 schreef Ville Syrjälä: > >>>>> On Wed, May 03, 2017 at 04:06:37PM +0200, Maarten Lankhorst wrote: > >>>>>> Op 03-05-17 om 15:45 schreef Ville Syrjälä: > >>>>>>> On Mon, May 01, 2017 at 03:34:34PM +0200, Maarten Lankhorst wrote: > >>>>>>>> The watermarks it should calculate against are the old optimal watermarks. > >>>>>>>> The currently active crtc watermarks are pure fiction, and are invalid in > >>>>>>>> case of a nonblocking modeset, page flip enabling/disabling planes or any > >>>>>>>> other reason. > >>>>>>>> > >>>>>>>> When the crtc is disabled or during a modeset the intermediate watermarks > >>>>>>>> don't need to be programmed separately, and could be directly assigned > >>>>>>>> to the optimal watermarks. > >>>>>>>> > >>>>>>>> Also rename crtc_state to new_crtc_state, to distinguish it from the old state. > >>>>>>>> > >>>>>>>> Changes since v1: > >>>>>>>> - Use intel_atomic_get_old_crtc_state. (ville) > >>>>>>>> > >>>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>>>>>>> --- > >>>>>>>> drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++------ > >>>>>>>> 1 file changed, 14 insertions(+), 6 deletions(-) > >>>>>>>> > >>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > >>>>>>>> index 0f344b1fff45..a09396ee1f3d 100644 > >>>>>>>> --- a/drivers/gpu/drm/i915/intel_pm.c > >>>>>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c > >>>>>>>> @@ -1458,16 +1458,24 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state, > >>>>>>>> > >>>>>>>> static int vlv_compute_intermediate_wm(struct drm_device *dev, > >>>>>>>> struct intel_crtc *crtc, > >>>>>>>> - struct intel_crtc_state *crtc_state) > >>>>>>>> + struct intel_crtc_state *new_crtc_state) > >>>>>>>> { > >>>>>>>> - struct vlv_wm_state *intermediate = &crtc_state->wm.vlv.intermediate; > >>>>>>>> - const struct vlv_wm_state *optimal = &crtc_state->wm.vlv.optimal; > >>>>>>>> - const struct vlv_wm_state *active = &crtc->wm.active.vlv; > >>>>>>>> + struct vlv_wm_state *intermediate = &new_crtc_state->wm.vlv.intermediate; > >>>>>>>> + const struct vlv_wm_state *optimal = &new_crtc_state->wm.vlv.optimal; > >>>>>>>> + const struct intel_crtc_state *old_crtc_state = > >>>>>>>> + intel_atomic_get_old_crtc_state(new_crtc_state->base.state, crtc); > >>>>>>>> + const struct vlv_wm_state *active = &old_crtc_state->wm.vlv.optimal; > >>>>>>>> int level; > >>>>>>>> > >>>>>>>> + if (!new_crtc_state->base.active || drm_atomic_crtc_needs_modeset(&new_crtc_state->base)) { > >>>>>>>> + *intermediate = *optimal; > >>>>>>>> + > >>>>>>>> + return 0; > >>>>>>>> + } > >>>>>>>> + > >>>>>>>> intermediate->num_levels = min(optimal->num_levels, active->num_levels); > >>>>>>>> intermediate->cxsr = optimal->cxsr && active->cxsr && > >>>>>>>> - !crtc_state->disable_cxsr; > >>>>>>>> + !new_crtc_state->disable_cxsr; > >>>>>>> We need to consider disable_cxsr even in the modeset case. > >>>>>> Why is this? crtc_state->disable_cxsr is set if any plane is part of the crtc during modeset, so it's disabled during modeset already. > >>>>> It's set if any plane is enabling/disabling, which should be quite > >>>>> typical during a modeset. > >>>> Yeah but .initial_watermarks is called during crtc_enable, so cxsr will get enabled anyway. > >>> Which is not what we want. CxSR must stay off until the planes have been > >>> enabled. > >>> > >> In that case why is it enabled in .initial_watermarks at all? It should be in optimize_watermarks then.. > > Because we can keep it enabled across the update unless planes are > > getting enabled or disabled. > > > So for the modeset case, computing intermediate watermarks: > > *intermediate = *optimal; > if (needs_modeset) > intermediate->cxsr = false; > > if (optimal->cxsr && !intermediate->cxsr) > new_crtc_state->wm.need_postvbl_update = true; > > ? Or maybe if (blah) { *intermediate = *optimal; goto out; } // min/max stuff out: if (disable_cxsr) intermediate->cxsr = false; if (memcmp(...
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 0f344b1fff45..a09396ee1f3d 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -1458,16 +1458,24 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state, static int vlv_compute_intermediate_wm(struct drm_device *dev, struct intel_crtc *crtc, - struct intel_crtc_state *crtc_state) + struct intel_crtc_state *new_crtc_state) { - struct vlv_wm_state *intermediate = &crtc_state->wm.vlv.intermediate; - const struct vlv_wm_state *optimal = &crtc_state->wm.vlv.optimal; - const struct vlv_wm_state *active = &crtc->wm.active.vlv; + struct vlv_wm_state *intermediate = &new_crtc_state->wm.vlv.intermediate; + const struct vlv_wm_state *optimal = &new_crtc_state->wm.vlv.optimal; + const struct intel_crtc_state *old_crtc_state = + intel_atomic_get_old_crtc_state(new_crtc_state->base.state, crtc); + const struct vlv_wm_state *active = &old_crtc_state->wm.vlv.optimal; int level; + if (!new_crtc_state->base.active || drm_atomic_crtc_needs_modeset(&new_crtc_state->base)) { + *intermediate = *optimal; + + return 0; + } + intermediate->num_levels = min(optimal->num_levels, active->num_levels); intermediate->cxsr = optimal->cxsr && active->cxsr && - !crtc_state->disable_cxsr; + !new_crtc_state->disable_cxsr; for (level = 0; level < intermediate->num_levels; level++) { enum plane_id plane_id; @@ -1491,7 +1499,7 @@ static int vlv_compute_intermediate_wm(struct drm_device *dev, * omit the post-vblank programming; only update if it's different. */ if (memcmp(intermediate, optimal, sizeof(*intermediate)) != 0) - crtc_state->wm.need_postvbl_update = true; + new_crtc_state->wm.need_postvbl_update = true; return 0; }
The watermarks it should calculate against are the old optimal watermarks. The currently active crtc watermarks are pure fiction, and are invalid in case of a nonblocking modeset, page flip enabling/disabling planes or any other reason. When the crtc is disabled or during a modeset the intermediate watermarks don't need to be programmed separately, and could be directly assigned to the optimal watermarks. Also rename crtc_state to new_crtc_state, to distinguish it from the old state. Changes since v1: - Use intel_atomic_get_old_crtc_state. (ville) Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)