From: Leo Li <[email protected]> # Abstract
The purpose of this series is to introduce a way for drivers to do blocking work as part of their vblank enable/disable callbacks. Today, this isn't possible since drm_crtc_funcs->(enable|disable)_vblank() can be called from atomic context. The proposed solution is to defer drm_vblank_enable() and vblank_disable_fn() to a workqueue from within the DRM vblank core, along with introducing new crtc callbacks where drivers can do blocking work. A drm-misc-next based branch with this series applied is available here: https://gitlab.freedesktop.org/leoli/drm-misc/-/commits/drm-misc-next # Considerations Why not defer within the driver callbacks? It can introduce concurrency between the deferred work, and access to HW upon the callback's return. See drm_vblank_enable()'s call to drm_update_vblank_count() that reads the HW vblank counter and scanout position, for example. If the underlying HW remains accessible when vblanks are disabled, then this wouldn't be an issue. But that's not always the case (amdgpu_dm being an example, see patch 4/5's commit description). One may be tempted to spin-wait on the driver's enable worker to complete from the callbacks that access HW, (*)but waiting on a deferred process-context worker while possibly in atomic context is not ideal. Doesn't deferring from the DRM vblank core have the same issue? Yes and no (and here is where I think some additional review is required): Since the entirety of drm_vblank_enable/disable_and_save() is deferred, access to HW counter and scanout position from within is synchronized with the enable/disable callbacks. However, it is still possible for the caller of drm_vblank_get() to run concurrently with the deferred enable worker that it scheduled. This is not an issue with vblank_put(), since HW is already in an enabled state, and it's OK for HW to disabled a little later. In cases where the vblank_getter wants to ensure that vblank counts will increment (e.g. for waiting on a specific vblank), this shouldn't be an issue: HW vblanks will be enabled eventually, and the counter will progress (albeit with some additional delay). This seems the case for all vblank_get() calls from within DRM core, with one exception addressed in patch 1/5. But if the vblank_getter requires HW to be enabled upon return (e.g. programming something that depends on HW being enabled), a new drm_crtc_vblank_wait_deferred_enable() helper is provided. Drivers can use it to wait for the enable work to complete before proceeding. This doesn't have the same concern as (*) above, since wait_deferred_enable() must run from process context. And if the driver needs deferred enable/disable work, it sounds reasonable to ask it to also do such work from process context. # Some more context This is actually a redo of a previous attempt to introduce a "vblank_prepare()" driver callback that the DRM core calls prior to entering atomic context. It was dropped because it has synchronization issues -- there was nothing preventing a previous vblank_put() from "undoing" the prepare work while there is a pending vblank_get() (IOW vblank_prepare() and vblank_disable() runs concurrently)(*). It also asks all callers of vblank_get() to be vblank_prepare()-aware, which isn't great. This series can be found here: https://lore.kernel.org/dri-devel/[email protected]/ (*) With the pre/post_enable/disable() callbacks for deferred vblank enable/disable, drivers can acquire/release their own locks, as long as they're properly paired. Leo Li (5): drm/vblank: Add drm_crtc_vblank_is_off() helper drm/vblank: Introduce deferred vblank enable/disable drm/amd/display: Refactor amdgpu_dm_crtc_set_vblank drm/amd/display: Implement deferred vblanks on IPS platforms drm/vblank: Add some debugging trace events .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 + .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c | 263 ++++++++++++++++-- .../amd/display/amdgpu_dm/amdgpu_dm_crtc.h | 4 - drivers/gpu/drm/drm_atomic_helper.c | 11 +- drivers/gpu/drm/drm_drv.c | 5 + drivers/gpu/drm/drm_internal.h | 1 + drivers/gpu/drm/drm_trace.h | 112 ++++++++ drivers/gpu/drm/drm_vblank.c | 227 ++++++++++++++- include/drm/drm_crtc.h | 34 +++ include/drm/drm_device.h | 6 + include/drm/drm_vblank.h | 20 ++ 11 files changed, 642 insertions(+), 49 deletions(-) -- 2.52.0
