[PATCH 0/2] drm/vkms: Bugfix for igt-tests

[PATCH 0/2] drm/vkms: Bugfix for igt-tests

[Shayenne Moura]

This patchset contains patches to fix the extra frame bug on kms_flip 
igt-test. First patch solves the extra vblank frame that breaks many
tests on kms_flip and second patch solves the race condition caused
by the solution added in the first one.

Shayenne Moura (2):
  drm/vkms: Bugfix extra vblank frame
  drm/vkms: Bugfix racing hrtimer vblank handle

 drivers/gpu/drm/vkms/vkms_crtc.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

-- 
2.17.1

[PATCH 1/2] drm/vkms: Bugfix extra vblank frame

[Shayenne Moura]

kms_flip tests are breaking on vkms when simulate vblank because vblank
event sequence count returns one extra frame after arm vblank event to
make a page flip.

When vblank interrupt happens, userspace processes the vblank event and
issues the next page flip command. Kernel calls queue_work to call
commit_planes and arm the new page flip. The next vblank picks up the
newly armed vblank event and vblank interrupt happens again.

The arm and vblank event are asynchronous, then, on the next vblank, we
receive x+2 from `get_vblank_timestamp`, instead x+1, although timestamp
and vblank seqno matches.

Function `get_vblank_timestamp` is reached by 2 ways:

  - from `drm_mode_page_flip_ioctl`: driver is doing one atomic operation
    to synchronize planes in the same output. There is no vblank simulation,
    the `drm_crtc_arm_vblank_event` function adds 1 on vblank count, and the
    variable in_vblank_irq is false
  - from `vkms_vblank_simulate`: since the driver is doing a vblank simulation,
    the variable in_vblank_irq is true.

Fix this problem subtracting one vblank period from vblank_time when
`get_vblank_timestamp` is called from trace `drm_mode_page_flip_ioctl`,
i.e., is not a real vblank interrupt, and getting the timestamp and vblank
seqno when it is a real vblank interrupt.

The reason for all this is that get_vblank_timestamp always supplies the
timestamp for the next vblank event. The hrtimer is the vblank simulator,
and it needs the correct previous value to present the next vblank. Since
this is how hw timestamp registers work and what the vblank core expects.

Signed-off-by: Shayenne Moura <shayenneluzmoura@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

---
 drivers/gpu/drm/vkms/vkms_crtc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index d44bfc392491..23146ff2a25b 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -87,6 +87,9 @@ bool vkms_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
 
 	*vblank_time = output->vblank_hrtimer.node.expires;
 
+	if (!in_vblank_irq)
+		*vblank_time -= output->period_ns;
+
 	return true;
 }
 
-- 
2.17.1

[PATCH 2/2] drm: vkms: Bugfix racing hrtimer vblank handle

[Shayenne Moura]

When the vblank irq happens, kernel time subsystem executes
`vkms_vblank_simulate`. In parallel or not, it prepares all stuff
necessary to the next vblank with arm, and it must flush these
stuff before the next vblank irq. However, vblank counter is ahead
when arm is executed in parallel with handle vblank.

CPU 0:					CPU 1:
 |					 |
atomic_commit_tail is ongoing		 |
 |					 |
 |					hrtimer: vkms_vblank_simulate()
 |					 |
 |					drm_crtc_handle_vblank()
 |					 |
drm_crtc_arm_vblank()			 |
 |					 |
->get_vblank_timestamp()		 |
 |					 |
 |					hrtimer_forward_now()

Then, we should guarantee that the vblank interval time is correct
(not changed) before finish the vblank handle.

Fix the bug including the call to `hrtimer_forward_now()` in the same
lock of `drm_crtc_handle_vblank()` to ensure that the timestamp update
is correct when finish the vblank handle.

Signed-off-by: Shayenne Moura <shayenneluzmoura@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/vkms/vkms_crtc.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 23146ff2a25b..5a095610726b 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -10,13 +10,17 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_probe_helper.h>
 
-static void _vblank_handle(struct vkms_output *output)
+static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
 {
+	struct vkms_output *output = container_of(timer, struct vkms_output,
+						  vblank_hrtimer);
 	struct drm_crtc *crtc = &output->crtc;
 	struct vkms_crtc_state *state = to_vkms_crtc_state(crtc->state);
+	int ret_overrun;
 	bool ret;
 
 	spin_lock(&output->lock);
+
 	ret = drm_crtc_handle_vblank(crtc);
 	if (!ret)
 		DRM_ERROR("vkms failure on handling vblank");
@@ -37,19 +41,9 @@ static void _vblank_handle(struct vkms_output *output)
 			DRM_WARN("failed to queue vkms_crc_work_handle");
 	}
 
-	spin_unlock(&output->lock);
-}
-
-static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
-{
-	struct vkms_output *output = container_of(timer, struct vkms_output,
-						  vblank_hrtimer);
-	int ret_overrun;
-
-	_vblank_handle(output);
-
 	ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
 					  output->period_ns);
+	spin_unlock(&output->lock);
 
 	return HRTIMER_RESTART;
 }
-- 
2.17.1

Re: [PATCH 0/2] drm/vkms: Bugfix for igt-tests

[Shayenne Moura]

Daniel Vetter and I were discussing about this solution. We figured out that
after these patches, tests were passing but when the computer has a heavy
background workload, tests fail.

I tried a new solution. Instead of change the vblank_time variable, make the
`get_vblank_timestamp` return false when is not happening a vblank_irq.

It worked in the same way of our last attempt.

The reason for this change is that, when the timestamp is not accurate,
drm_vblank deals with this correcting timestamp when `get_vblank_timestamp`
returns false. However, this requirement is important when real hardware has an
error, and could not return the accurate vblank. Now, we need to know if our
`vkms_get_vblank_timestamp` should return false and when.

Em qua, 30 de jan de 2019 às 14:05, Shayenne Moura
<shayenneluzmoura@gmail.com> escreveu:
>
> This patchset contains patches to fix the extra frame bug on kms_flip
> igt-test. First patch solves the extra vblank frame that breaks many
> tests on kms_flip and second patch solves the race condition caused
> by the solution added in the first one.
>
> Shayenne Moura (2):
>   drm/vkms: Bugfix extra vblank frame
>   drm/vkms: Bugfix racing hrtimer vblank handle
>
>  drivers/gpu/drm/vkms/vkms_crtc.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
>
> --
> 2.17.1
>

Re: [PATCH 0/2] drm/vkms: Bugfix for igt-tests

[Rodrigo Siqueira]

[-- Attachment #1: Type: text/plain, Size: 898 bytes --]

On 01/30, Shayenne Moura wrote:
> This patchset contains patches to fix the extra frame bug on kms_flip 
> igt-test. First patch solves the extra vblank frame that breaks many
> tests on kms_flip and second patch solves the race condition caused
> by the solution added in the first one.
> 
> Shayenne Moura (2):
>   drm/vkms: Bugfix extra vblank frame
>   drm/vkms: Bugfix racing hrtimer vblank handle
> 
>  drivers/gpu/drm/vkms/vkms_crtc.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> -- 
> 2.17.1
> 

Hi,

Thanks for the patchset :)

The patchset worked like a charm; it fixes many of the tests in the
kms_flip. \o/

I'll apply it.

Thanks!

Reviewed-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>

-- 
Rodrigo Siqueira
https://siqueira.tech
Graduate Student
Department of Computer Science
University of São Paulo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] drm/vkms: Bugfix extra vblank frame

[Rodrigo Siqueira]

[-- Attachment #1: Type: text/plain, Size: 2627 bytes --]

On 01/30, Shayenne Moura wrote:
> kms_flip tests are breaking on vkms when simulate vblank because vblank
> event sequence count returns one extra frame after arm vblank event to
> make a page flip.
> 
> When vblank interrupt happens, userspace processes the vblank event and
> issues the next page flip command. Kernel calls queue_work to call
> commit_planes and arm the new page flip. The next vblank picks up the
> newly armed vblank event and vblank interrupt happens again.
> 
> The arm and vblank event are asynchronous, then, on the next vblank, we
> receive x+2 from `get_vblank_timestamp`, instead x+1, although timestamp
> and vblank seqno matches.
> 
> Function `get_vblank_timestamp` is reached by 2 ways:
> 
>   - from `drm_mode_page_flip_ioctl`: driver is doing one atomic operation
>     to synchronize planes in the same output. There is no vblank simulation,
>     the `drm_crtc_arm_vblank_event` function adds 1 on vblank count, and the
>     variable in_vblank_irq is false
>   - from `vkms_vblank_simulate`: since the driver is doing a vblank simulation,
>     the variable in_vblank_irq is true.
> 
> Fix this problem subtracting one vblank period from vblank_time when
> `get_vblank_timestamp` is called from trace `drm_mode_page_flip_ioctl`,
> i.e., is not a real vblank interrupt, and getting the timestamp and vblank
> seqno when it is a real vblank interrupt.
> 
> The reason for all this is that get_vblank_timestamp always supplies the
> timestamp for the next vblank event. The hrtimer is the vblank simulator,
> and it needs the correct previous value to present the next vblank. Since
> this is how hw timestamp registers work and what the vblank core expects.
> 
> Signed-off-by: Shayenne Moura <shayenneluzmoura@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> ---
>  drivers/gpu/drm/vkms/vkms_crtc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index d44bfc392491..23146ff2a25b 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -87,6 +87,9 @@ bool vkms_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
>  
>  	*vblank_time = output->vblank_hrtimer.node.expires;
>  
> +	if (!in_vblank_irq)
> +		*vblank_time -= output->period_ns;
> +
>  	return true;
>  }
>  
> -- 
> 2.17.1
> 

Reviewed-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>

-- 
Rodrigo Siqueira
https://siqueira.tech
Graduate Student
Department of Computer Science
University of São Paulo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] drm: vkms: Bugfix racing hrtimer vblank handle

[Rodrigo Siqueira]

[-- Attachment #1: Type: text/plain, Size: 2910 bytes --]

On 01/30, Shayenne Moura wrote:
> When the vblank irq happens, kernel time subsystem executes
> `vkms_vblank_simulate`. In parallel or not, it prepares all stuff
> necessary to the next vblank with arm, and it must flush these
> stuff before the next vblank irq. However, vblank counter is ahead
> when arm is executed in parallel with handle vblank.
> 
> CPU 0:					CPU 1:
>  |					 |
> atomic_commit_tail is ongoing		 |
>  |					 |
>  |					hrtimer: vkms_vblank_simulate()
>  |					 |
>  |					drm_crtc_handle_vblank()
>  |					 |
> drm_crtc_arm_vblank()			 |
>  |					 |
> ->get_vblank_timestamp()		 |
>  |					 |
>  |					hrtimer_forward_now()
> 
> Then, we should guarantee that the vblank interval time is correct
> (not changed) before finish the vblank handle.
> 
> Fix the bug including the call to `hrtimer_forward_now()` in the same
> lock of `drm_crtc_handle_vblank()` to ensure that the timestamp update
> is correct when finish the vblank handle.
> 
> Signed-off-by: Shayenne Moura <shayenneluzmoura@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/vkms/vkms_crtc.c | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 23146ff2a25b..5a095610726b 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -10,13 +10,17 @@
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_probe_helper.h>
>  
> -static void _vblank_handle(struct vkms_output *output)
> +static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
>  {
> +	struct vkms_output *output = container_of(timer, struct vkms_output,
> +						  vblank_hrtimer);
>  	struct drm_crtc *crtc = &output->crtc;
>  	struct vkms_crtc_state *state = to_vkms_crtc_state(crtc->state);
> +	int ret_overrun;
>  	bool ret;
>  
>  	spin_lock(&output->lock);
> +
>  	ret = drm_crtc_handle_vblank(crtc);
>  	if (!ret)
>  		DRM_ERROR("vkms failure on handling vblank");
> @@ -37,19 +41,9 @@ static void _vblank_handle(struct vkms_output *output)
>  			DRM_WARN("failed to queue vkms_crc_work_handle");
>  	}
>  
> -	spin_unlock(&output->lock);
> -}
> -
> -static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> -{
> -	struct vkms_output *output = container_of(timer, struct vkms_output,
> -						  vblank_hrtimer);
> -	int ret_overrun;
> -
> -	_vblank_handle(output);
> -
>  	ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
>  					  output->period_ns);
> +	spin_unlock(&output->lock);
>  
>  	return HRTIMER_RESTART;
>  }
> -- 
> 2.17.1
> 

Reviewed-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>

-- 
Rodrigo Siqueira
https://siqueira.tech
Graduate Student
Department of Computer Science
University of São Paulo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]