[(resend),1/2] drm/i915: Calculate vlv/chv intermediate watermarks correctly, v3.
diff mbox

Message ID 20171115163157.14372-1-maarten.lankhorst@linux.intel.com
State New
Headers show

Commit Message

Maarten Lankhorst Nov. 15, 2017, 4:31 p.m. UTC
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.

CXSR must always be disabled in the intermediate case for modesets, else
we get a WARN for vblank wait timeout.

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)
Changes since v2:
- Always unset cxsr during modeset.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

Comments

Ville Syrjälä Nov. 17, 2017, 1:31 p.m. UTC | #1
On Wed, Nov 15, 2017 at 05:31:56PM +0100, 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.
> 
> CXSR must always be disabled in the intermediate case for modesets, else
> we get a WARN for vblank wait timeout.
> 
> 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)
> Changes since v2:
> - Always unset cxsr during modeset.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

I was going to try and figure out how/if these get rid of the unclaimed
reg warns, but I didn't quite get that far. I did spot a few other
buglets in the wm code though (I'll send fixes for those at some point).

Anyways, these patches make sense to me, so for the series
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 8c69ec9eb6ee..f904bf73dbd6 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2026,16 +2026,27 @@ 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;
> +	struct intel_atomic_state *intel_state =
> +		to_intel_atomic_state(new_crtc_state->base.state);
> +	const struct intel_crtc_state *old_crtc_state =
> +		intel_atomic_get_old_crtc_state(intel_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;
> +
> +		intermediate->cxsr = false;
> +		goto out;
> +	}
> +
>  	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;
> @@ -2054,12 +2065,13 @@ static int vlv_compute_intermediate_wm(struct drm_device *dev,
>  
>  	vlv_invalidate_wms(crtc, intermediate, level);
>  
> +out:
>  	/*
>  	 * If our intermediate WM are identical to the final WM, then we can
>  	 * 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.15.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Maarten Lankhorst Nov. 17, 2017, 2:47 p.m. UTC | #2
Op 17-11-17 om 14:31 schreef Ville Syrjälä:
> On Wed, Nov 15, 2017 at 05:31:56PM +0100, 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.
>>
>> CXSR must always be disabled in the intermediate case for modesets, else
>> we get a WARN for vblank wait timeout.
>>
>> 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)
>> Changes since v2:
>> - Always unset cxsr during modeset.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> I was going to try and figure out how/if these get rid of the unclaimed
> reg warns, but I didn't quite get that far. I did spot a few other
> buglets in the wm code though (I'll send fixes for those at some point).
>
> Anyways, these patches make sense to me, so for the series
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Seems Chris Wilson already beat us to it..

https://intel-gfx-ci.01.org/tree/drm-tip/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b.html
was gone with

commit 1a1f12872edcd5e425b668a35fb23548cfa918ef
Author: Chris Wilson <chris@chris-wilson.co.uk <mailto:chris@chris-wilson.co.uk>>
Date:   Tue Nov 7 14:03:38 2017 +0000

    drm/i915: Prevent unbounded wm results in g4x_compute_wm()

But I think this patch also fixes it in a slightly different way, never using garbage for intermediaries. :-)

Thanks for review, pushed.

Cheers,
~Maarten
Ville Syrjälä Nov. 17, 2017, 2:53 p.m. UTC | #3
On Fri, Nov 17, 2017 at 03:47:58PM +0100, Maarten Lankhorst wrote:
> Op 17-11-17 om 14:31 schreef Ville Syrjälä:
> > On Wed, Nov 15, 2017 at 05:31:56PM +0100, 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.
> >>
> >> CXSR must always be disabled in the intermediate case for modesets, else
> >> we get a WARN for vblank wait timeout.
> >>
> >> 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)
> >> Changes since v2:
> >> - Always unset cxsr during modeset.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > I was going to try and figure out how/if these get rid of the unclaimed
> > reg warns, but I didn't quite get that far. I did spot a few other
> > buglets in the wm code though (I'll send fixes for those at some point).
> >
> > Anyways, these patches make sense to me, so for the series
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Seems Chris Wilson already beat us to it..
> 
> https://intel-gfx-ci.01.org/tree/drm-tip/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b.html
> was gone with
> 
> commit 1a1f12872edcd5e425b668a35fb23548cfa918ef
> Author: Chris Wilson <chris@chris-wilson.co.uk <mailto:chris@chris-wilson.co.uk>>
> Date:   Tue Nov 7 14:03:38 2017 +0000
> 
>     drm/i915: Prevent unbounded wm results in g4x_compute_wm()

That patch should be nop. So this is a very surprising result.

> 
> But I think this patch also fixes it in a slightly different way, never using garbage for intermediaries. :-)
> 
> Thanks for review, pushed.
> 
> Cheers,
> ~Maarten
Maarten Lankhorst Nov. 17, 2017, 3 p.m. UTC | #4
Op 17-11-17 om 15:53 schreef Ville Syrjälä:
> On Fri, Nov 17, 2017 at 03:47:58PM +0100, Maarten Lankhorst wrote:
>> Op 17-11-17 om 14:31 schreef Ville Syrjälä:
>>> On Wed, Nov 15, 2017 at 05:31:56PM +0100, 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.
>>>>
>>>> CXSR must always be disabled in the intermediate case for modesets, else
>>>> we get a WARN for vblank wait timeout.
>>>>
>>>> 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)
>>>> Changes since v2:
>>>> - Always unset cxsr during modeset.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> I was going to try and figure out how/if these get rid of the unclaimed
>>> reg warns, but I didn't quite get that far. I did spot a few other
>>> buglets in the wm code though (I'll send fixes for those at some point).
>>>
>>> Anyways, these patches make sense to me, so for the series
>>> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Seems Chris Wilson already beat us to it..
>>
>> https://intel-gfx-ci.01.org/tree/drm-tip/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b.html
>> was gone with
>>
>> commit 1a1f12872edcd5e425b668a35fb23548cfa918ef
>> Author: Chris Wilson <chris@chris-wilson.co.uk <mailto:chris@chris-wilson.co.uk>>
>> Date:   Tue Nov 7 14:03:38 2017 +0000
>>
>>     drm/i915: Prevent unbounded wm results in g4x_compute_wm()
> That patch should be nop. So this is a very surprising result.
>
Oh well maybe the watermarks initially read out were being garbage. Garbage in, garbage out. :-)

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 8c69ec9eb6ee..f904bf73dbd6 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2026,16 +2026,27 @@  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;
+	struct intel_atomic_state *intel_state =
+		to_intel_atomic_state(new_crtc_state->base.state);
+	const struct intel_crtc_state *old_crtc_state =
+		intel_atomic_get_old_crtc_state(intel_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;
+
+		intermediate->cxsr = false;
+		goto out;
+	}
+
 	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;
@@ -2054,12 +2065,13 @@  static int vlv_compute_intermediate_wm(struct drm_device *dev,
 
 	vlv_invalidate_wms(crtc, intermediate, level);
 
+out:
 	/*
 	 * If our intermediate WM are identical to the final WM, then we can
 	 * 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;
 }