From d63716658ac16c515d1223a9fbf5edbf76b1b333 Mon Sep 17 00:00:00 2001 From: Mario Kleiner Date: Wed, 30 Jan 2019 06:04:46 +0100 Subject: [PATCH 1/3] drm/amd/display: Use vrr friendly pageflip throttling in DC. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In VRR mode, keep track of the vblank count of the last completed pageflip in amdgpu_crtc->last_flip_vblank, as recorded in the pageflip completion handler after each completed flip. Use that count to prevent mmio programming a new pageflip within the same vblank in which the last pageflip completed, iow. to throttle pageflips to at most one flip per video frame, while at the same time allowing to request a flip not only before start of vblank, but also anywhere within vblank. The old logic did the same, and made sense for regular fixed refresh rate flipping, but in vrr mode it prevents requesting a flip anywhere inside the possibly huge vblank, thereby reducing framerate in vrr mode instead of improving it, by delaying a slightly delayed flip requests up to a maximum vblank duration + 1 scanout duration. This would limit VRR usefulness to only help applications with a very high GPU demand, which can submit the flip request before start of vblank, but then have to wait long for fences to complete. With this method a flip can be both requested and - after fences have completed - executed, ie. it doesn't matter if the request (amdgpu_dm_do_flip()) gets delayed until deep into the extended vblank due to cpu execution delays. This also allows clients which want to regulate framerate within the vrr range a much more fine-grained control of flip timing, a feature that might be useful for video playback, and is very useful for neuroscience/vision research applications. In regular non-VRR mode, retain the old flip submission behavior. This to keep flip scheduling for fullscreen X11/GLX OpenGL clients intact, if they use the GLX_OML_sync_control extensions glXSwapBufferMscOML(, ..., target_msc,...) function with a specific target_msc target vblank count. glXSwapBuffersMscOML() or DRI3/Present PresentPixmap() will not flip at the proper target_msc for a non-zero target_msc if VRR mode is active with this patch. They'd often flip one frame too early. However, this limitation should not matter much in VRR mode, as scheduling based on vblank counts is pretty futile/unusable under variable refresh duration anyway, so no real extra harm is done. Signed-off-by: Mario Kleiner Cc: Nicholas Kazlauskas Cc: Harry Wentland Cc: Alex Deucher Cc: Michel Dänzer Signed-off-by: Alex Deucher --- drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 1 + .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 27 ++++++++++++++++--- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h index aadd0fa42e43..3aa42c64484a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h @@ -405,6 +405,7 @@ struct amdgpu_crtc { struct amdgpu_flip_work *pflip_works; enum amdgpu_flip_status pflip_status; int deferred_flip_completion; + u64 last_flip_vblank; /* pll sharing */ struct amdgpu_atom_ss ss; bool ss_enabled; diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 5296b8f3e0ab..636d14a60952 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -303,12 +303,11 @@ static void dm_pflip_high_irq(void *interrupt_params) return; } + /* Update to correct count(s) if racing with vblank irq */ + amdgpu_crtc->last_flip_vblank = drm_crtc_accurate_vblank_count(&amdgpu_crtc->base); /* wake up userspace */ if (amdgpu_crtc->event) { - /* Update to correct count(s) if racing with vblank irq */ - drm_crtc_accurate_vblank_count(&amdgpu_crtc->base); - drm_crtc_send_vblank_event(&amdgpu_crtc->base, amdgpu_crtc->event); /* page flip completed. clean up */ @@ -4828,6 +4827,8 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state, to_dm_crtc_state(drm_atomic_get_old_crtc_state(state, pcrtc)); int planes_count = 0; unsigned long flags; + u64 last_flip_vblank; + bool vrr_active = acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE; /* update planes when needed */ for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) { @@ -4859,6 +4860,16 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state, /* In commit tail framework this cannot happen */ WARN_ON(1); } + + /* For variable refresh rate mode only: + * Get vblank of last completed flip to avoid > 1 vrr flips per + * video frame by use of throttling, but allow flip programming + * anywhere in the possibly large variable vrr vblank interval + * for fine-grained flip timing control and more opportunity to + * avoid stutter on late submission of amdgpu_dm_do_flip() calls. + */ + last_flip_vblank = acrtc_attach->last_flip_vblank; + spin_unlock_irqrestore(&crtc->dev->event_lock, flags); if (!pflip_needed || plane->type == DRM_PLANE_TYPE_OVERLAY) { @@ -4882,10 +4893,18 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state, if (plane->type == DRM_PLANE_TYPE_PRIMARY) drm_crtc_vblank_get(crtc); + /* Use old throttling in non-vrr fixed refresh rate mode + * to keep flip scheduling based on target vblank counts + * working in a backwards compatible way, e.g., clients + * using GLX_OML_sync_control extension. + */ + if (!vrr_active) + last_flip_vblank = drm_crtc_vblank_count(crtc); + amdgpu_dm_do_flip( crtc, fb, - (uint32_t)drm_crtc_vblank_count(crtc) + *wait_for_vblank, + (uint32_t) last_flip_vblank + *wait_for_vblank, dc_state); } From 2216322919c8608a448d7ebc560a845238a5d6b6 Mon Sep 17 00:00:00 2001 From: Nicholas Kazlauskas Date: Mon, 7 Jan 2019 12:41:46 -0500 Subject: [PATCH 2/3] drm: Block fb changes for async plane updates The prepare_fb call always happens on new_plane_state. The drm_atomic_helper_cleanup_planes checks to see if plane state pointer has changed when deciding to call cleanup_fb on either the new_plane_state or the old_plane_state. For a non-async atomic commit the state pointer is swapped, so this helper calls prepare_fb on the new_plane_state and cleanup_fb on the old_plane_state. This makes sense, since we want to prepare the framebuffer we are going to use and cleanup the the framebuffer we are no longer using. For the async atomic update helpers this differs. The async atomic update helpers perform in-place updates on the existing state. They call drm_atomic_helper_cleanup_planes but the state pointer is not swapped. This means that prepare_fb is called on the new_plane_state and cleanup_fb is called on the new_plane_state (not the old). In the case where old_plane_state->fb == new_plane_state->fb then there should be no behavioral difference between an async update and a non-async commit. But there are issues that arise when old_plane_state->fb != new_plane_state->fb. The first is that the new_plane_state->fb is immediately cleaned up after it has been prepared, so we're using a fb that we shouldn't be. The second occurs during a sequence of async atomic updates and non-async regular atomic commits. Suppose there are two framebuffers being interleaved in a double-buffering scenario, fb1 and fb2: - Async update, oldfb = NULL, newfb = fb1, prepare fb1, cleanup fb1 - Async update, oldfb = fb1, newfb = fb2, prepare fb2, cleanup fb2 - Non-async commit, oldfb = fb2, newfb = fb1, prepare fb1, cleanup fb2 We call cleanup_fb on fb2 twice in this example scenario, and any further use will result in use-after-free. The simple fix to this problem is to block framebuffer changes in the drm_atomic_helper_async_check function for now. v2: Move check by itself, add a FIXME (Daniel) Cc: Daniel Vetter Cc: Harry Wentland Cc: Andrey Grodzovsky Cc: # v4.14+ Fixes: fef9df8b5945 ("drm/atomic: initial support for asynchronous plane update") Signed-off-by: Nicholas Kazlauskas Acked-by: Andrey Grodzovsky Acked-by: Harry Wentland Reviewed-by: Daniel Vetter Signed-off-by: Harry Wentland Link: https://patchwork.freedesktop.org/patch/275364/ Signed-off-by: Dave Airlie --- drivers/gpu/drm/drm_atomic_helper.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 54e2ae614dcc..f4290f6b0c38 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1602,6 +1602,15 @@ int drm_atomic_helper_async_check(struct drm_device *dev, old_plane_state->crtc != new_plane_state->crtc) return -EINVAL; + /* + * FIXME: Since prepare_fb and cleanup_fb are always called on + * the new_plane_state for async updates we need to block framebuffer + * changes. This prevents use of a fb that's been cleaned up and + * double cleanups from occuring. + */ + if (old_plane_state->fb != new_plane_state->fb) + return -EINVAL; + funcs = plane->helper_private; if (!funcs->atomic_async_update) return -EINVAL; From 17fb465f16027ea22225b282295f5b8af19992e0 Mon Sep 17 00:00:00 2001 From: Alistair Francis Date: Thu, 21 Feb 2019 00:33:03 +0000 Subject: [PATCH 3/3] drm/bochs: Fix the ID mismatch error When running RISC-V QEMU with the Bochs device attached via PCIe the probe of the Bochs device fails with: [drm:bochs_hw_init] *ERROR* ID mismatch This was introduced by this commit: 7780eb9ce8 bochs: convert to drm_dev_register To fix the error we ensure that pci_enable_device() is called before bochs_load(). Fixes: 7780eb9ce80f ("bochs: convert to drm_dev_register") Signed-off-by: Alistair Francis Reported-by: David Abdurachmanov Link: http://patchwork.freedesktop.org/patch/msgid/20190221003231.31625-1-alistair.francis@wdc.com Signed-off-by: Gerd Hoffmann Signed-off-by: Dave Airlie --- drivers/gpu/drm/bochs/bochs_drv.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/bochs/bochs_drv.c b/drivers/gpu/drm/bochs/bochs_drv.c index f3dd66ae990a..aa35007262cd 100644 --- a/drivers/gpu/drm/bochs/bochs_drv.c +++ b/drivers/gpu/drm/bochs/bochs_drv.c @@ -154,6 +154,10 @@ static int bochs_pci_probe(struct pci_dev *pdev, if (IS_ERR(dev)) return PTR_ERR(dev); + ret = pci_enable_device(pdev); + if (ret) + goto err_free_dev; + dev->pdev = pdev; pci_set_drvdata(pdev, dev);