[PATCH 1/2] drm/i915: Move updating color management to before vblank evasion

[PATCH 1/2] drm/i915: Move updating color management to before vblank evasion

[Maarten Lankhorst]

This cannot be done reliably during vblank evasasion
since the color management registers are not double buffered.

The original commit that moved it always during vblank evasion was
wrong, so revert it to before vblank evasion again.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: 20a34e78f0d7 ("drm/i915: Update color management during vblank evasion.")
Cc: stable@vger.kernel.org # v4.7+
---
 drivers/gpu/drm/i915/intel_display.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3c862d0ab389..a0108041fd4a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13301,17 +13301,19 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 		to_intel_atomic_state(old_crtc_state->state);
 	bool modeset = needs_modeset(crtc->state);
 
+	if (!modeset &&
+	    (intel_cstate->base.color_mgmt_changed ||
+	     intel_cstate->update_pipe)) {
+		intel_color_set_csc(crtc->state);
+		intel_color_load_luts(crtc->state);
+	}
+
 	/* Perform vblank evasion around commit operation */
 	intel_pipe_update_start(intel_crtc);
 
 	if (modeset)
 		goto out;
 
-	if (crtc->state->color_mgmt_changed || to_intel_crtc_state(crtc->state)->update_pipe) {
-		intel_color_set_csc(crtc->state);
-		intel_color_load_luts(crtc->state);
-	}
-
 	if (intel_cstate->update_pipe)
 		intel_update_pipe_config(intel_crtc, old_intel_cstate);
 	else if (INTEL_GEN(dev_priv) >= 9)
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: Complain if we take too long under vblank evasion.

[Maarten Lankhorst]

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

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 27e0752d1578..375ca91b308c 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -65,6 +65,8 @@ int intel_usecs_to_scanlines(const struct drm_display_mode *adjusted_mode,
 			    1000 * adjusted_mode->crtc_htotal);
 }
 
+#define VBLANK_EVASION_TIME_US 100
+
 /**
  * intel_pipe_update_start() - start update of a set of display registers
  * @crtc: the crtc of which the registers are going to be updated
@@ -92,7 +94,8 @@ void intel_pipe_update_start(struct intel_crtc *crtc)
 		vblank_start = DIV_ROUND_UP(vblank_start, 2);
 
 	/* FIXME needs to be calibrated sensibly */
-	min = vblank_start - intel_usecs_to_scanlines(adjusted_mode, 100);
+	min = vblank_start - intel_usecs_to_scanlines(adjusted_mode,
+						      VBLANK_EVASION_TIME_US);
 	max = vblank_start - 1;
 
 	local_irq_disable();
@@ -191,7 +194,12 @@ void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work
 			  ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time),
 			  crtc->debug.min_vbl, crtc->debug.max_vbl,
 			  crtc->debug.scanline_start, scanline_end);
-	}
+	} else if (ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time) >
+		   VBLANK_EVASION_TIME_US)
+		DRM_WARN("Atomic update on pipe (%c) took %lld us, max time under evasion is %u us\n",
+			 pipe_name(pipe),
+			 ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time),
+			 VBLANK_EVASION_TIME_US);
 }
 
 static void
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Move updating color management to before vblank evasion

[Ville Syrjälä]

On Tue, Feb 28, 2017 at 03:28:47PM +0100, Maarten Lankhorst wrote:
> This cannot be done reliably during vblank evasasion
> since the color management registers are not double buffered.
> 
> The original commit that moved it always during vblank evasion was
> wrong, so revert it to before vblank evasion again.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Fixes: 20a34e78f0d7 ("drm/i915: Update color management during vblank evasion.")
> Cc: stable@vger.kernel.org # v4.7+

Wasn't there a bugzilla + tested-by ?

> ---
>  drivers/gpu/drm/i915/intel_display.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3c862d0ab389..a0108041fd4a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13301,17 +13301,19 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>  		to_intel_atomic_state(old_crtc_state->state);
>  	bool modeset = needs_modeset(crtc->state);
>  
> +	if (!modeset &&
> +	    (intel_cstate->base.color_mgmt_changed ||
> +	     intel_cstate->update_pipe)) {
> +		intel_color_set_csc(crtc->state);
> +		intel_color_load_luts(crtc->state);
> +	}
> +
>  	/* Perform vblank evasion around commit operation */
>  	intel_pipe_update_start(intel_crtc);
>  
>  	if (modeset)
>  		goto out;
>  
> -	if (crtc->state->color_mgmt_changed || to_intel_crtc_state(crtc->state)->update_pipe) {
> -		intel_color_set_csc(crtc->state);
> -		intel_color_load_luts(crtc->state);
> -	}
> -
>  	if (intel_cstate->update_pipe)
>  		intel_update_pipe_config(intel_crtc, old_intel_cstate);
>  	else if (INTEL_GEN(dev_priv) >= 9)
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Move updating color management to before vblank evasion

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Move updating color management to before vblank evasion
URL   : https://patchwork.freedesktop.org/series/20399/
State : success

== Summary ==

Series 20399v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/20399/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> FAIL       (fi-snb-2600) fdo#100007
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                skip       -> INCOMPLETE (fi-bsw-n3050) fdo#100004

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#100004 https://bugs.freedesktop.org/show_bug.cgi?id=100004

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:236  pass:201  dwarn:0   dfail:0   fail:0   skip:34 
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:108  pass:95   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29 
fi-byt-j1900 failed to collect. IGT log at Patchwork_4006/fi-byt-j1900/igt.log

5d37006b578e38562382215e8782cfced9c992ce drm-tip: 2017y-02m-28d-16h-27m-13s UTC integration manifest
5bced1e drm/i915: Complain if we take too long under vblank evasion.
628d35b drm/i915: Move updating color management to before vblank evasion

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4006/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Move updating color management to before vblank evasion

[Ville Syrjälä]

On Tue, Feb 28, 2017 at 05:01:25PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 28, 2017 at 03:28:47PM +0100, Maarten Lankhorst wrote:
> > This cannot be done reliably during vblank evasasion
> > since the color management registers are not double buffered.
> > 
> > The original commit that moved it always during vblank evasion was
> > wrong, so revert it to before vblank evasion again.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Fixes: 20a34e78f0d7 ("drm/i915: Update color management during vblank evasion.")
> > Cc: stable@vger.kernel.org # v4.7+
> 
> Wasn't there a bugzilla + tested-by ?

Seems we have two bugzillas now:
https://bugs.freedesktop.org/show_bug.cgi?id=91883
https://bugs.freedesktop.org/show_bug.cgi?id=99991

First one is about the atomic update fails, the second about
the assert_dsi_pll() mutex vs. irq_disabled() warning.

No actual t-bs for this specific variant of the patch (yet)
from what I can see.

Anyways, this is 
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 3c862d0ab389..a0108041fd4a 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13301,17 +13301,19 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
> >  		to_intel_atomic_state(old_crtc_state->state);
> >  	bool modeset = needs_modeset(crtc->state);
> >  
> > +	if (!modeset &&
> > +	    (intel_cstate->base.color_mgmt_changed ||
> > +	     intel_cstate->update_pipe)) {
> > +		intel_color_set_csc(crtc->state);
> > +		intel_color_load_luts(crtc->state);
> > +	}
> > +
> >  	/* Perform vblank evasion around commit operation */
> >  	intel_pipe_update_start(intel_crtc);
> >  
> >  	if (modeset)
> >  		goto out;
> >  
> > -	if (crtc->state->color_mgmt_changed || to_intel_crtc_state(crtc->state)->update_pipe) {
> > -		intel_color_set_csc(crtc->state);
> > -		intel_color_load_luts(crtc->state);
> > -	}
> > -
> >  	if (intel_cstate->update_pipe)
> >  		intel_update_pipe_config(intel_crtc, old_intel_cstate);
> >  	else if (INTEL_GEN(dev_priv) >= 9)
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Complain if we take too long under vblank evasion.

[Ville Syrjälä]

On Tue, Feb 28, 2017 at 03:28:48PM +0100, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 27e0752d1578..375ca91b308c 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -65,6 +65,8 @@ int intel_usecs_to_scanlines(const struct drm_display_mode *adjusted_mode,
>  			    1000 * adjusted_mode->crtc_htotal);
>  }
>  
> +#define VBLANK_EVASION_TIME_US 100
> +
>  /**
>   * intel_pipe_update_start() - start update of a set of display registers
>   * @crtc: the crtc of which the registers are going to be updated
> @@ -92,7 +94,8 @@ void intel_pipe_update_start(struct intel_crtc *crtc)
>  		vblank_start = DIV_ROUND_UP(vblank_start, 2);
>  
>  	/* FIXME needs to be calibrated sensibly */
> -	min = vblank_start - intel_usecs_to_scanlines(adjusted_mode, 100);
> +	min = vblank_start - intel_usecs_to_scanlines(adjusted_mode,
> +						      VBLANK_EVASION_TIME_US);
>  	max = vblank_start - 1;
>  
>  	local_irq_disable();
> @@ -191,7 +194,12 @@ void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work
>  			  ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time),
>  			  crtc->debug.min_vbl, crtc->debug.max_vbl,
>  			  crtc->debug.scanline_start, scanline_end);
> -	}
> +	} else if (ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time) >
> +		   VBLANK_EVASION_TIME_US)
> +		DRM_WARN("Atomic update on pipe (%c) took %lld us, max time under evasion is %u us\n",
> +			 pipe_name(pipe),
> +			 ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time),
> +			 VBLANK_EVASION_TIME_US);

Hmm. Yeah this should make it very likely that we'll catch any
potential fails even when they don't happen to hit the actual critical
section. Hopefully it won't result in massive amounts of noise. One way
to find out ;)

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  }
>  
>  static void
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Complain if we take too long under vblank evasion.

[Maarten Lankhorst]

Op 01-03-17 om 16:40 schreef Ville Syrjälä:
> On Tue, Feb 28, 2017 at 03:28:48PM +0100, Maarten Lankhorst wrote:
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_sprite.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>> index 27e0752d1578..375ca91b308c 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -65,6 +65,8 @@ int intel_usecs_to_scanlines(const struct drm_display_mode *adjusted_mode,
>>  			    1000 * adjusted_mode->crtc_htotal);
>>  }
>>  
>> +#define VBLANK_EVASION_TIME_US 100
>> +
>>  /**
>>   * intel_pipe_update_start() - start update of a set of display registers
>>   * @crtc: the crtc of which the registers are going to be updated
>> @@ -92,7 +94,8 @@ void intel_pipe_update_start(struct intel_crtc *crtc)
>>  		vblank_start = DIV_ROUND_UP(vblank_start, 2);
>>  
>>  	/* FIXME needs to be calibrated sensibly */
>> -	min = vblank_start - intel_usecs_to_scanlines(adjusted_mode, 100);
>> +	min = vblank_start - intel_usecs_to_scanlines(adjusted_mode,
>> +						      VBLANK_EVASION_TIME_US);
>>  	max = vblank_start - 1;
>>  
>>  	local_irq_disable();
>> @@ -191,7 +194,12 @@ void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work
>>  			  ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time),
>>  			  crtc->debug.min_vbl, crtc->debug.max_vbl,
>>  			  crtc->debug.scanline_start, scanline_end);
>> -	}
>> +	} else if (ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time) >
>> +		   VBLANK_EVASION_TIME_US)
>> +		DRM_WARN("Atomic update on pipe (%c) took %lld us, max time under evasion is %u us\n",
>> +			 pipe_name(pipe),
>> +			 ktime_us_delta(end_vbl_time, crtc->debug.start_vbl_time),
>> +			 VBLANK_EVASION_TIME_US);
> Hmm. Yeah this should make it very likely that we'll catch any
> potential fails even when they don't happen to hit the actual critical
> section. Hopefully it won't result in massive amounts of noise. One way
> to find out ;)
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Thanks, pushed. Also added a commit message to this commit.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx