[PATCH 1/3] drm: Zero out invalid vblank timestamp in drm_update_vblank_count.

[PATCH 1/3] drm: Zero out invalid vblank timestamp in drm_update_vblank_count.

[Mario Kleiner]

Since commit 844b03f27739135fe1fed2fef06da0ffc4c7a081 we make
sure that after vblank irq off, we return the last valid
(vblank count, vblank timestamp) pair to clients, e.g., during
modesets, which is good.

An overlooked side effect of that commit for kms drivers without
support for precise vblank timestamping is that at vblank irq
enable, when we update the vblank counter from the hw counter, we
can't update the corresponding vblank timestamp, so now we have a
totally mismatched timestamp for the new count to confuse clients.

Restore old client visible behaviour from before Linux 3.17, but
zero out the timestamp at vblank counter update (instead of disable
as in original implementation) if we can't generate a meaningful
timestamp immediately for the new vblank counter. This will fix
this regression, so callers know they need to retry again later
if they need a valid timestamp, but at the same time preserves
the improvements made in the commit mentioned above.

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
Cc: <stable@vger.kernel.org> #v3.17+

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_irq.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index c8a3447..af9662e 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -131,12 +131,11 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
 
 	/* Reinitialize corresponding vblank timestamp if high-precision query
 	 * available. Skip this step if query unsupported or failed. Will
-	 * reinitialize delayed at next vblank interrupt in that case.
+	 * reinitialize delayed at next vblank interrupt in that case and
+	 * assign 0 for now, to mark the vblanktimestamp as invalid.
 	 */
-	if (rc) {
-		tslot = atomic_read(&vblank->count) + diff;
-		vblanktimestamp(dev, crtc, tslot) = t_vblank;
-	}
+	tslot = atomic_read(&vblank->count) + diff;
+	vblanktimestamp(dev, crtc, tslot) = rc ? t_vblank : (struct timeval) {0, 0};
 
 	smp_mb__before_atomic();
 	atomic_add(diff, &vblank->count);
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm: Prevent invalid use of vblank_disable_immediate.

[Mario Kleiner]

For a kms driver to support immediate disable of vblank
irq's reliably without introducing off by one errors or
other mayhem for clients, it must not only support a
hardware vblank counter query, but also high precision
vblank timestamping, so vblank count and timestamp can be
instantaneously reinitialzed to valid values. Additionally
the exposed hardware counter must behave as if it is
incrementing at leading edge of vblank to avoid off by
one errors during reinitialization of the counter while
the display happens to be inside or close to vblank.

Check during drm_vblank_init that a driver which claims to
be capable of vblank_disable_immediate at least supports
high precision timestamping and prevent use of instant
disable if that isn't present as a minimum requirement.

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Michel Dänzer <michel@daenzer.net>
Cc: Thierry Reding <treding@nvidia.com>
Cc: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_irq.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index af9662e..6efe822 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -336,6 +336,12 @@ int drm_vblank_init(struct drm_device *dev, int num_crtcs)
 	else
 		DRM_INFO("No driver support for vblank timestamp query.\n");
 
+	/* Must have precise timestamping for reliable vblank instant disable */
+	if (dev->vblank_disable_immediate && !dev->driver->get_vblank_timestamp) {
+		dev->vblank_disable_immediate = false;
+		DRM_ERROR("Set vblank_disable_immediate, but not supported.\n");
+	}
+
 	dev->vblank_disable_allowed = false;
 
 	return 0;
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/tegra: Don't use vblank_disable_immediate on incapable driver.

[Mario Kleiner]

Tegra would not only need a hardware vblank counter that
increments at leading edge of vblank, but also support
for instantaneous high precision vblank timestamp queries, ie.
a proper implementation of dev->driver->get_vblank_timestamp().

Without these, there can be off-by-one errors during vblank
disable/enable if the scanout is inside vblank at en/disable
time, and additionally clients will never see any useable
vblank timestamps when querying via drmWaitVblank ioctl. This
would negatively affect swap scheduling under X11 and Wayland.

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
Cc: Thierry Reding <treding@nvidia.com>
Cc: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/tegra/drm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 1833abd..bfad15a 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -173,7 +173,6 @@ static int tegra_drm_load(struct drm_device *drm, unsigned long flags)
 	drm->irq_enabled = true;
 
 	/* syncpoints are used for full 32-bit hardware VBLANK counters */
-	drm->vblank_disable_immediate = true;
 	drm->max_vblank_count = 0xffffffff;
 
 	err = drm_vblank_init(drm, drm->mode_config.num_crtc);
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: Prevent invalid use of vblank_disable_immediate.

[Michel Dänzer]

On 15.04.2015 02:41, Mario Kleiner wrote:
> For a kms driver to support immediate disable of vblank
> irq's reliably without introducing off by one errors or
> other mayhem for clients, it must not only support a
> hardware vblank counter query, but also high precision
> vblank timestamping, so vblank count and timestamp can be
> instantaneously reinitialzed to valid values. Additionally
> the exposed hardware counter must behave as if it is
> incrementing at leading edge of vblank to avoid off by
> one errors during reinitialization of the counter while
> the display happens to be inside or close to vblank.
> 
> Check during drm_vblank_init that a driver which claims to
> be capable of vblank_disable_immediate at least supports
> high precision timestamping and prevent use of instant
> disable if that isn't present as a minimum requirement.
> 
> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Michel Dänzer <michel@daenzer.net>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/drm_irq.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index af9662e..6efe822 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -336,6 +336,12 @@ int drm_vblank_init(struct drm_device *dev, int num_crtcs)
>  	else
>  		DRM_INFO("No driver support for vblank timestamp query.\n");
>  
> +	/* Must have precise timestamping for reliable vblank instant disable */
> +	if (dev->vblank_disable_immediate && !dev->driver->get_vblank_timestamp) {
> +		dev->vblank_disable_immediate = false;
> +		DRM_ERROR("Set vblank_disable_immediate, but not supported.\n");
> +	}

I think DRM_ERROR is kind of a bad compromise for this. If this is
considered a driver bug, something like WARN_ONCE would be better to
draw attention to the culprit. Otherwise, maybe something like

DRM_INFO("Setting vblank_disable_immediate to false because "
	 "get_vblank_timestamp == NULL\n");

would be both clearer and less alarming.

Other than that, looks good to me.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel