Re: [PATCH] drm/i915: suppress atomic commit error message under gvt-g env

Re: [PATCH] drm/i915: suppress atomic commit error message under gvt-g env

[Zhi Wang]

Can we directly use DRM_DEBUG_KMS() for this periodic error message?

On 03/03/17 19:53, bing.niu@intel.com wrote:
> From: Bing Niu <bing.niu@intel.com>
>
> under virtualization enviroment, it is possible guest update pipe
> registers across vblank intervals due to overhead of mmio traps or vm
> schedule out. However, it is safe since those pipe update happen in
> virual registers and will not be committed to hardware. suppress that
> atomic commit error message under virtualization case to avoid
> confusing user.
>
> Signed-off-by: Bing Niu <bing.niu@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_sprite.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index b16a295..5ce1ec6 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -158,6 +158,7 @@ void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work
>   	int scanline_end = intel_get_crtc_scanline(crtc);
>   	u32 end_vbl_count = intel_crtc_get_vblank_counter(crtc);
>   	ktime_t end_vbl_time = ktime_get();
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>
>   	if (work) {
>   		work->flip_queued_vblank = end_vbl_count;
> @@ -184,7 +185,7 @@ void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work
>   	local_irq_enable();
>
>   	if (crtc->debug.start_vbl_count &&
> -	    crtc->debug.start_vbl_count != end_vbl_count) {
> +	    crtc->debug.start_vbl_count != end_vbl_count && !intel_vgpu_active(dev_priv)) {
>   		DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u) time %lld us, min %d, max %d, scanline start %d, end %d\n",
>   			  pipe_name(pipe), crtc->debug.start_vbl_count,
>   			  end_vbl_count,
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for drm/i915: suppress atomic commit error message under gvt-g env

[Patchwork]

== Series Details ==

Series: drm/i915: suppress atomic commit error message under gvt-g env
URL   : https://patchwork.freedesktop.org/series/20600/
State : warning

== Summary ==

Series 20600v1 drm/i915: suppress atomic commit error message under gvt-g env
https://patchwork.freedesktop.org/api/1.0/series/20600/revisions/1/mbox/

Test drv_module_reload:
        Subgroup basic-reload:
                dmesg-warn -> PASS       (fi-ilk-650)
Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                fail       -> PASS       (fi-snb-2600) fdo#100007
Test kms_force_connector_basic:
        Subgroup prune-stale-modes:
                pass       -> SKIP       (fi-snb-2600)
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                pass       -> DMESG-WARN (fi-byt-n2820)

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

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20 
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:278  pass:246  dwarn:1   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:0   skip:30 

7f38bb9ea553c613223b705326c1b8d1a8fc4a90 drm-tip: 2017y-03m-02d-22h-35m-41s UTC integration manifest
bae46b8 drm/i915: suppress atomic commit error message under gvt-g env

== Logs ==

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

Re: [PATCH] drm/i915: suppress atomic commit error message under gvt-g env

[Niu, Bing]

that depends on whether native can accept this log level degraded.
let's see if there another opinions  from community~

-----Original Message-----
From: Wang, Zhi A 
Sent: Friday, March 03, 2017 10:56 AM
To: Niu, Bing <bing.niu@intel.com>; intel-gfx@lists.freedesktop.org
Cc: Lv, Zhiyuan <zhiyuan.lv@intel.com>
Subject: Re: [Intel-gfx][PATCH] drm/i915: suppress atomic commit error message under gvt-g env

Can we directly use DRM_DEBUG_KMS() for this periodic error message?

On 03/03/17 19:53, bing.niu@intel.com wrote:
> From: Bing Niu <bing.niu@intel.com>
>
> under virtualization enviroment, it is possible guest update pipe 
> registers across vblank intervals due to overhead of mmio traps or vm 
> schedule out. However, it is safe since those pipe update happen in 
> virual registers and will not be committed to hardware. suppress that 
> atomic commit error message under virtualization case to avoid 
> confusing user.
>
> Signed-off-by: Bing Niu <bing.niu@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_sprite.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
> b/drivers/gpu/drm/i915/intel_sprite.c
> index b16a295..5ce1ec6 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -158,6 +158,7 @@ void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work
>   	int scanline_end = intel_get_crtc_scanline(crtc);
>   	u32 end_vbl_count = intel_crtc_get_vblank_counter(crtc);
>   	ktime_t end_vbl_time = ktime_get();
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>
>   	if (work) {
>   		work->flip_queued_vblank = end_vbl_count; @@ -184,7 +185,7 @@ void 
> intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work
>   	local_irq_enable();
>
>   	if (crtc->debug.start_vbl_count &&
> -	    crtc->debug.start_vbl_count != end_vbl_count) {
> +	    crtc->debug.start_vbl_count != end_vbl_count && 
> +!intel_vgpu_active(dev_priv)) {
>   		DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u) time %lld us, min %d, max %d, scanline start %d, end %d\n",
>   			  pipe_name(pipe), crtc->debug.start_vbl_count,
>   			  end_vbl_count,
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: suppress atomic commit error message under gvt-g env

[Ville Syrjälä]

On Fri, Mar 03, 2017 at 06:53:08AM -0500, bing.niu@intel.com wrote:
> From: Bing Niu <bing.niu@intel.com>
> 
> under virtualization enviroment, it is possible guest update pipe
> registers across vblank intervals due to overhead of mmio traps or vm
> schedule out. However, it is safe since those pipe update happen in
> virual registers and will not be committed to hardware.

Hmm. How are these virtual registers used? Do you present some kind of
virtualized display for the guest? And if this stuff fails does that
mean that guest will fail in doing atomic display updates?

> suppress that
> atomic commit error message under virtualization case to avoid
> confusing user.
> 
> Signed-off-by: Bing Niu <bing.niu@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index b16a295..5ce1ec6 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -158,6 +158,7 @@ void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work
>  	int scanline_end = intel_get_crtc_scanline(crtc);
>  	u32 end_vbl_count = intel_crtc_get_vblank_counter(crtc);
>  	ktime_t end_vbl_time = ktime_get();
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  
>  	if (work) {
>  		work->flip_queued_vblank = end_vbl_count;
> @@ -184,7 +185,7 @@ void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work
>  	local_irq_enable();
>  

if (vgpu_active())
	return;

here might look neater, and would still work with the additional error print
that Maarten is going to add [1]

[1] https://patchwork.freedesktop.org/patch/141260/

>  	if (crtc->debug.start_vbl_count &&
> -	    crtc->debug.start_vbl_count != end_vbl_count) {
> +	    crtc->debug.start_vbl_count != end_vbl_count && !intel_vgpu_active(dev_priv)) {
>  		DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u) time %lld us, min %d, max %d, scanline start %d, end %d\n",
>  			  pipe_name(pipe), crtc->debug.start_vbl_count,
>  			  end_vbl_count,
> -- 
> 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

[PATCH] drm/i915: suppress atomic commit error message under gvt-g env

[bing.niu]

From: Bing Niu <bing.niu@intel.com>

under virtualization enviroment, it is possible guest update pipe
registers across vblank intervals due to overhead of mmio traps or vm
schedule out. However, it is safe since those pipe update happen in
virual registers and will not be committed to hardware. suppress that
atomic commit error message under virtualization case to avoid
confusing user.

Signed-off-by: Bing Niu <bing.niu@intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index b16a295..5ce1ec6 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -158,6 +158,7 @@ void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work
 	int scanline_end = intel_get_crtc_scanline(crtc);
 	u32 end_vbl_count = intel_crtc_get_vblank_counter(crtc);
 	ktime_t end_vbl_time = ktime_get();
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 
 	if (work) {
 		work->flip_queued_vblank = end_vbl_count;
@@ -184,7 +185,7 @@ void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work
 	local_irq_enable();
 
 	if (crtc->debug.start_vbl_count &&
-	    crtc->debug.start_vbl_count != end_vbl_count) {
+	    crtc->debug.start_vbl_count != end_vbl_count && !intel_vgpu_active(dev_priv)) {
 		DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u) time %lld us, min %d, max %d, scanline start %d, end %d\n",
 			  pipe_name(pipe), crtc->debug.start_vbl_count,
 			  end_vbl_count,
-- 
2.7.4

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

Re: [PATCH] drm/i915: suppress atomic commit error message under gvt-g env

[Ville Syrjälä]

On Fri, Mar 03, 2017 at 10:56:16AM +0800, Zhi Wang wrote:
> Can we directly use DRM_DEBUG_KMS() for this periodic error message?

No. We want to actually know when/if this fails.

> 
> On 03/03/17 19:53, bing.niu@intel.com wrote:
> > From: Bing Niu <bing.niu@intel.com>
> >
> > under virtualization enviroment, it is possible guest update pipe
> > registers across vblank intervals due to overhead of mmio traps or vm
> > schedule out. However, it is safe since those pipe update happen in
> > virual registers and will not be committed to hardware. suppress that
> > atomic commit error message under virtualization case to avoid
> > confusing user.
> >
> > Signed-off-by: Bing Niu <bing.niu@intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_sprite.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index b16a295..5ce1ec6 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -158,6 +158,7 @@ void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work
> >   	int scanline_end = intel_get_crtc_scanline(crtc);
> >   	u32 end_vbl_count = intel_crtc_get_vblank_counter(crtc);
> >   	ktime_t end_vbl_time = ktime_get();
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >
> >   	if (work) {
> >   		work->flip_queued_vblank = end_vbl_count;
> > @@ -184,7 +185,7 @@ void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work
> >   	local_irq_enable();
> >
> >   	if (crtc->debug.start_vbl_count &&
> > -	    crtc->debug.start_vbl_count != end_vbl_count) {
> > +	    crtc->debug.start_vbl_count != end_vbl_count && !intel_vgpu_active(dev_priv)) {
> >   		DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u) time %lld us, min %d, max %d, scanline start %d, end %d\n",
> >   			  pipe_name(pipe), crtc->debug.start_vbl_count,
> >   			  end_vbl_count,
> >
> _______________________________________________
> 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] drm/i915: suppress atomic commit error message under gvt-g env

[Zhi Wang]

Hi Ville:
     Thanks for the reply!  :P

Is this error message for the end-user or for debug? If this is for the 
end-user, only warning one time should be better? I saw a case on 
Celeron, if this happens, usually this would happen again and agin, the 
error message could fill all the dmesg. If this is only for debug, 
should DRM_DEBUG_KMS() would be better?

Anyway, thanks for the reply! :P

Thanks,
Zhi.

于 03/03/17 19:53, Ville Syrjälä 写道:
> On Fri, Mar 03, 2017 at 10:56:16AM +0800, Zhi Wang wrote:
>> Can we directly use DRM_DEBUG_KMS() for this periodic error message?
>
> No. We want to actually know when/if this fails.
>
>>
>> On 03/03/17 19:53, bing.niu@intel.com wrote:
>>> From: Bing Niu <bing.niu@intel.com>
>>>
>>> under virtualization enviroment, it is possible guest update pipe
>>> registers across vblank intervals due to overhead of mmio traps or vm
>>> schedule out. However, it is safe since those pipe update happen in
>>> virual registers and will not be committed to hardware. suppress that
>>> atomic commit error message under virtualization case to avoid
>>> confusing user.
>>>
>>> Signed-off-by: Bing Niu <bing.niu@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/intel_sprite.c | 3 ++-
>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>>> index b16a295..5ce1ec6 100644
>>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>>> @@ -158,6 +158,7 @@ void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work
>>>    	int scanline_end = intel_get_crtc_scanline(crtc);
>>>    	u32 end_vbl_count = intel_crtc_get_vblank_counter(crtc);
>>>    	ktime_t end_vbl_time = ktime_get();
>>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>>
>>>    	if (work) {
>>>    		work->flip_queued_vblank = end_vbl_count;
>>> @@ -184,7 +185,7 @@ void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work
>>>    	local_irq_enable();
>>>
>>>    	if (crtc->debug.start_vbl_count &&
>>> -	    crtc->debug.start_vbl_count != end_vbl_count) {
>>> +	    crtc->debug.start_vbl_count != end_vbl_count && !intel_vgpu_active(dev_priv)) {
>>>    		DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u) time %lld us, min %d, max %d, scanline start %d, end %d\n",
>>>    			  pipe_name(pipe), crtc->debug.start_vbl_count,
>>>    			  end_vbl_count,
>>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: suppress atomic commit error message under gvt-g env

[bing.niu]

Hi ville:
thanks for the reply. :)
yeah! we present a virtual monitor to guest associating with those 
vregs. however, we use remote display protocol like x11vnc to display 
vm's desktop.

cheer~

-----Original Message-----
From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Friday, 
March 03, 2017 7:53 PM
To: Niu, Bing <bing.niu@intel.com>
Cc: intel-gfx@lists.freedesktop.org; Lv, Zhiyuan <zhiyuan.lv@intel.com>
Subject: Re: [Intel-gfx] [PATCH] drm/i915: suppress atomic commit error 
message under gvt-g env

On Fri, Mar 03, 2017 at 06:53:08AM -0500, bing.niu@intel.com wrote:
> From: Bing Niu <bing.niu@intel.com>
>
> under virtualization enviroment, it is possible guest update pipe
> registers across vblank intervals due to overhead of mmio traps or vm
> schedule out. However, it is safe since those pipe update happen in
> virual registers and will not be committed to hardware.

Hmm. How are these virtual registers used? Do you present some kind of 
virtualized display for the guest? And if this stuff fails does that 
mean that guest will fail in doing atomic display updates?

> suppress that
> atomic commit error message under virtualization case to avoid
> confusing user.
>
> Signed-off-by: Bing Niu <bing.niu@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index b16a295..5ce1ec6 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -158,6 +158,7 @@ void intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work
>  	int scanline_end = intel_get_crtc_scanline(crtc);
>  	u32 end_vbl_count = intel_crtc_get_vblank_counter(crtc);
>  	ktime_t end_vbl_time = ktime_get();
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>
>  	if (work) {
>  		work->flip_queued_vblank = end_vbl_count; @@ -184,7 +185,7 @@ void
> intel_pipe_update_end(struct intel_crtc *crtc, struct intel_flip_work *work
>  	local_irq_enable();
>

if (vgpu_active())
	return;

here might look neater, and would still work with the additional error 
print that Maarten is going to add [1]

[1] https://patchwork.freedesktop.org/patch/141260/

>  	if (crtc->debug.start_vbl_count &&
> -	    crtc->debug.start_vbl_count != end_vbl_count) {
> +	    crtc->debug.start_vbl_count != end_vbl_count &&
> +!intel_vgpu_active(dev_priv)) {
>  		DRM_ERROR("Atomic update failure on pipe %c (start=%u end=%u) time %lld us, min %d, max %d, scanline start %d, end %d\n",
>  			  pipe_name(pipe), crtc->debug.start_vbl_count,
>  			  end_vbl_count,
> --
> 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