Hi, a resend of updated versions of the earlier patches:
Patch 1 should really get into Linux 4.1 to avoid tegra breaking user-space clients.
Patch 2 reviewed-by Michel, updated to take his feedback into account.
Patch 3 is modified to not conflict with Daniel Vetter's patch "drm/vblank: Fixup and document timestamp update/read barriers"
Patch 4 will be needed for drm/qxl to compile with Daniel's fixup patch applied.
thanks, -mario
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);
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.
v2: Changed from DRM_ERROR to DRM_INFO and made message more clear, as suggested by Michel Dänzer.
Signed-off-by: Mario Kleiner mario.kleiner.de@gmail.com Reviewed-by: Michel Dänzer michel.daenzer@amd.com
Cc: Dave Airlie airlied@redhat.com --- drivers/gpu/drm/drm_irq.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 9c166b4..152d1de 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -352,6 +352,13 @@ 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_INFO("Setting vblank_disable_immediate to false because " + "get_vblank_timestamp == NULL\n"); + } + dev->vblank_disable_allowed = false;
return 0;
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.18, 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.
v2: Rebased on top of Daniel Vetter's fixup and documentation patch for timestamp updates. Drop request for stable kernel backport as this would be more difficult, unless the original patch would get applied to stable kernels.
Signed-off-by: Mario Kleiner mario.kleiner.de@gmail.com
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, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 152d1de..44e6a20b 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -161,10 +161,13 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
/* * Only reinitialize corresponding vblank timestamp if high-precision query - * available and didn't fail. Will reinitialize delayed at next vblank - * interrupt in that case. + * available and didn't fail. Otherwise reinitialize delayed at next vblank + * interrupt and assign 0 for now, to mark the vblanktimestamp as invalid. */ - store_vblank(dev, crtc, diff, rc ? &t_vblank : NULL); + if (!rc) + t_vblank = (struct timeval) {0, 0}; + + store_vblank(dev, crtc, diff, &t_vblank); }
/*
This breaks under the vblank timestamp cleanup patch by Daniel Vetter. Also it is pointless to return anything but zero (or any other constant) if the function doesn't actually query a hw vblank counter. The bogus return of the current drm vblank counter via direct readout or via drm_vblank_count() is found in many of the new kms drivers, but it does exactly nothing different from returning any arbitrary constant - it's a no operation.
Let's simply return 0 - Easy and fast.
Signed-off-by: Mario Kleiner mario.kleiner.de@gmail.com Cc: Dave Airlie airlied@redhat.com --- drivers/gpu/drm/qxl/qxl_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c index 1d9b80c..577dc45 100644 --- a/drivers/gpu/drm/qxl/qxl_drv.c +++ b/drivers/gpu/drm/qxl/qxl_drv.c @@ -198,7 +198,7 @@ static int qxl_pm_restore(struct device *dev)
static u32 qxl_noop_get_vblank_counter(struct drm_device *dev, int crtc) { - return dev->vblank[crtc].count.counter; + return 0; }
static int qxl_noop_enable_vblank(struct drm_device *dev, int crtc)
On Mon, May 04, 2015 at 06:29:43AM +0200, Mario Kleiner wrote:
Hi, a resend of updated versions of the earlier patches:
Patch 1 should really get into Linux 4.1 to avoid tegra breaking user-space clients.
Patch 2 reviewed-by Michel, updated to take his feedback into account.
Patch 3 is modified to not conflict with Daniel Vetter's patch "drm/vblank: Fixup and document timestamp update/read barriers"
Patch 4 will be needed for drm/qxl to compile with Daniel's fixup patch applied.
Merged patch 2-4 to topic/drm-misc on top of my memory barrier patch. I'll leave 1 to Thierry for tegra-fixes. And there doesn't seem to be a patch 5 somehow, was that intentional? The patches are numbered n/5. -Daniel
On 05/04/2015 11:15 AM, Daniel Vetter wrote:
On Mon, May 04, 2015 at 06:29:43AM +0200, Mario Kleiner wrote:
Hi, a resend of updated versions of the earlier patches:
Patch 1 should really get into Linux 4.1 to avoid tegra breaking user-space clients.
Patch 2 reviewed-by Michel, updated to take his feedback into account.
Patch 3 is modified to not conflict with Daniel Vetter's patch "drm/vblank: Fixup and document timestamp update/read barriers"
Patch 4 will be needed for drm/qxl to compile with Daniel's fixup patch applied.
Merged patch 2-4 to topic/drm-misc on top of my memory barrier patch. I'll leave 1 to Thierry for tegra-fixes. And there doesn't seem to be a patch 5 somehow, was that intentional? The patches are numbered n/5. -Daniel
Thanks. There isn't a patch 5, that was just some hack of mine that accidentally slipped into the numbering.
-mario
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Mario Kleiner