[(resend),1/3] drm/i915: Ignore previous watermarks on ILK if inherited
diff mbox

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

Commit Message

Maarten Lankhorst Nov. 8, 2017, 9:29 a.m. UTC
Fixes the following error when fastset is enabled, caught by CI:

[drm:ilk_validate_wm_level.part.8 [i915]] Sprite WM0 too large 56 (max 0)
[drm:ilk_validate_pipe_wm [i915]] LP0 watermark invalid
[drm:intel_crtc_atomic_check [i915]] No valid intermediate pipe watermarks are possible

Triggered on debugfs_test.read_all_entries, but could have been any igt
test depending on ordering.

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

Comments

Daniel Vetter Nov. 8, 2017, 3:41 p.m. UTC | #1
On Wed, Nov 08, 2017 at 10:29:20AM +0100, Maarten Lankhorst wrote:
> Fixes the following error when fastset is enabled, caught by CI:
> 
> [drm:ilk_validate_wm_level.part.8 [i915]] Sprite WM0 too large 56 (max 0)
> [drm:ilk_validate_pipe_wm [i915]] LP0 watermark invalid
> [drm:intel_crtc_atomic_check [i915]] No valid intermediate pipe watermarks are possible
> 
> Triggered on debugfs_test.read_all_entries, but could have been any igt
> test depending on ordering.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

So I'm no expert on this, but why is this not needed for g4x and vlv
intermediate wm? I think the commit message should explain that. I think
it should also explain why simply shutting the above warnings up is safe.

I think once that's fixed I understand why we need this and why it works,
but probably better to get an Ack from Ville to make sure.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 46440e2ecb33..9e8a0a9cac02 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3137,7 +3137,9 @@ static int ilk_compute_intermediate_wm(struct drm_device *dev,
>  	 * and after the vblank.
>  	 */
>  	*a = newstate->wm.ilk.optimal;
> -	if (!newstate->base.active || drm_atomic_crtc_needs_modeset(&newstate->base))
> +	if (!newstate->base.active ||
> +	    drm_atomic_crtc_needs_modeset(&newstate->base) ||
> +	    oldstate->base.mode.private_flags & I915_MODE_FLAG_INHERITED)
>  		return 0;
>  
>  	a->pipe_enabled |= b->pipe_enabled;
> -- 
> 2.15.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Maarten Lankhorst Nov. 8, 2017, 4:09 p.m. UTC | #2
Op 08-11-17 om 16:41 schreef Daniel Vetter:
> On Wed, Nov 08, 2017 at 10:29:20AM +0100, Maarten Lankhorst wrote:
>> Fixes the following error when fastset is enabled, caught by CI:
>>
>> [drm:ilk_validate_wm_level.part.8 [i915]] Sprite WM0 too large 56 (max 0)
>> [drm:ilk_validate_pipe_wm [i915]] LP0 watermark invalid
>> [drm:intel_crtc_atomic_check [i915]] No valid intermediate pipe watermarks are possible
>>
>> Triggered on debugfs_test.read_all_entries, but could have been any igt
>> test depending on ordering.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> So I'm no expert on this, but why is this not needed for g4x and vlv
> intermediate wm? I think the commit message should explain that. I think
> it should also explain why simply shutting the above warnings up is safe.
>
> I think once that's fixed I understand why we need this and why it works,
> but probably better to get an Ack from Ville to make sure.
> -Daniel
True, reason it's not needed there is because intermediate calculation can't fail.
And yeah best say because BIOS wm are bogus here.
>> ---
>>  drivers/gpu/drm/i915/intel_pm.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 46440e2ecb33..9e8a0a9cac02 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -3137,7 +3137,9 @@ static int ilk_compute_intermediate_wm(struct drm_device *dev,
>>  	 * and after the vblank.
>>  	 */
>>  	*a = newstate->wm.ilk.optimal;
>> -	if (!newstate->base.active || drm_atomic_crtc_needs_modeset(&newstate->base))
>> +	if (!newstate->base.active ||
>> +	    drm_atomic_crtc_needs_modeset(&newstate->base) ||
>> +	    oldstate->base.mode.private_flags & I915_MODE_FLAG_INHERITED)
>>  		return 0;
>>  
>>  	a->pipe_enabled |= b->pipe_enabled;
>> -- 
>> 2.15.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter Nov. 9, 2017, 10:41 a.m. UTC | #3
On Wed, Nov 08, 2017 at 05:09:33PM +0100, Maarten Lankhorst wrote:
> Op 08-11-17 om 16:41 schreef Daniel Vetter:
> > On Wed, Nov 08, 2017 at 10:29:20AM +0100, Maarten Lankhorst wrote:
> >> Fixes the following error when fastset is enabled, caught by CI:
> >>
> >> [drm:ilk_validate_wm_level.part.8 [i915]] Sprite WM0 too large 56 (max 0)
> >> [drm:ilk_validate_pipe_wm [i915]] LP0 watermark invalid
> >> [drm:intel_crtc_atomic_check [i915]] No valid intermediate pipe watermarks are possible
> >>
> >> Triggered on debugfs_test.read_all_entries, but could have been any igt
> >> test depending on ordering.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > So I'm no expert on this, but why is this not needed for g4x and vlv
> > intermediate wm? I think the commit message should explain that. I think
> > it should also explain why simply shutting the above warnings up is safe.
> >
> > I think once that's fixed I understand why we need this and why it works,
> > but probably better to get an Ack from Ville to make sure.
> > -Daniel
> True, reason it's not needed there is because intermediate calculation can't fail.
> And yeah best say because BIOS wm are bogus here.

Ok, with that (maybe a bit more verbose) explained in the commit message:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> >> ---
> >>  drivers/gpu/drm/i915/intel_pm.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >> index 46440e2ecb33..9e8a0a9cac02 100644
> >> --- a/drivers/gpu/drm/i915/intel_pm.c
> >> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >> @@ -3137,7 +3137,9 @@ static int ilk_compute_intermediate_wm(struct drm_device *dev,
> >>  	 * and after the vblank.
> >>  	 */
> >>  	*a = newstate->wm.ilk.optimal;
> >> -	if (!newstate->base.active || drm_atomic_crtc_needs_modeset(&newstate->base))
> >> +	if (!newstate->base.active ||
> >> +	    drm_atomic_crtc_needs_modeset(&newstate->base) ||
> >> +	    oldstate->base.mode.private_flags & I915_MODE_FLAG_INHERITED)
> >>  		return 0;
> >>  
> >>  	a->pipe_enabled |= b->pipe_enabled;
> >> -- 
> >> 2.15.0
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
>
Maarten Lankhorst Nov. 9, 2017, 2:20 p.m. UTC | #4
Op 09-11-17 om 11:41 schreef Daniel Vetter:
> On Wed, Nov 08, 2017 at 05:09:33PM +0100, Maarten Lankhorst wrote:
>> Op 08-11-17 om 16:41 schreef Daniel Vetter:
>>> On Wed, Nov 08, 2017 at 10:29:20AM +0100, Maarten Lankhorst wrote:
>>>> Fixes the following error when fastset is enabled, caught by CI:
>>>>
>>>> [drm:ilk_validate_wm_level.part.8 [i915]] Sprite WM0 too large 56 (max 0)
>>>> [drm:ilk_validate_pipe_wm [i915]] LP0 watermark invalid
>>>> [drm:intel_crtc_atomic_check [i915]] No valid intermediate pipe watermarks are possible
>>>>
>>>> Triggered on debugfs_test.read_all_entries, but could have been any igt
>>>> test depending on ordering.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> So I'm no expert on this, but why is this not needed for g4x and vlv
>>> intermediate wm? I think the commit message should explain that. I think
>>> it should also explain why simply shutting the above warnings up is safe.
>>>
>>> I think once that's fixed I understand why we need this and why it works,
>>> but probably better to get an Ack from Ville to make sure.
>>> -Daniel
>> True, reason it's not needed there is because intermediate calculation can't fail.
>> And yeah best say because BIOS wm are bogus here.
> Ok, with that (maybe a bit more verbose) explained in the commit message:
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Hm, something fishy is going on. This should have been caught by sanitize_watermarks() early during boot. More investigation is needed...

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 46440e2ecb33..9e8a0a9cac02 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3137,7 +3137,9 @@  static int ilk_compute_intermediate_wm(struct drm_device *dev,
 	 * and after the vblank.
 	 */
 	*a = newstate->wm.ilk.optimal;
-	if (!newstate->base.active || drm_atomic_crtc_needs_modeset(&newstate->base))
+	if (!newstate->base.active ||
+	    drm_atomic_crtc_needs_modeset(&newstate->base) ||
+	    oldstate->base.mode.private_flags & I915_MODE_FLAG_INHERITED)
 		return 0;
 
 	a->pipe_enabled |= b->pipe_enabled;