| Message ID | 20200115121652.7050-1-tzimmermann@suse.de |
|---|---|
| Headers | show |
| Series | drm: Clean up VBLANK callbacks in struct drm_driver | expand |
On Wed, Jan 15, 2020 at 01:16:33PM +0100, Thomas Zimmermann wrote: > VBLANK interrupts can be disabled immediately or with a delay, where the > latter is the default. The former option can be selected by setting > get_vblank_timestamp, and enabling vblank_disable_immediate in struct > drm_device. > > The setup is only evaluated once when DRM initializes VBLANKs. Evaluating > the settings on each use of vblank_disable_immediate will allow for easy > integration of CRTC VBLANK functions. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/drm_vblank.c | 31 ++++++++++++++----------------- > 1 file changed, 14 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c > index 3f1dd54cc8bb..abb085c67d82 100644 > --- a/drivers/gpu/drm/drm_vblank.c > +++ b/drivers/gpu/drm/drm_vblank.c > @@ -481,19 +481,6 @@ int drm_vblank_init(struct drm_device *dev, unsigned int num_crtcs) > > DRM_INFO("Supports vblank timestamp caching Rev 2 (21.10.2013).\n"); > > - /* Driver specific high-precision vblank timestamping supported? */ > - if (dev->driver->get_vblank_timestamp) > - DRM_INFO("Driver supports precise vblank timestamp query.\n"); > - 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"); > - } Which drivers are so broken they set vblank_disable_immediate to true without having the vfunc specified? IMO this code should just go away (or converted to a WARN). > - > return 0; > > err: > @@ -1070,6 +1057,15 @@ int drm_crtc_vblank_get(struct drm_crtc *crtc) > } > EXPORT_SYMBOL(drm_crtc_vblank_get); > > +static bool __vblank_disable_immediate(struct drm_device *dev, unsigned int pipe) > +{ > + if (!dev->vblank_disable_immediate) > + return false; > + if (!dev->driver->get_vblank_timestamp) > + return false; > + return true; > +} > + > static void drm_vblank_put(struct drm_device *dev, unsigned int pipe) > { > struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; > @@ -1086,7 +1082,7 @@ static void drm_vblank_put(struct drm_device *dev, unsigned int pipe) > return; > else if (drm_vblank_offdelay < 0) > vblank_disable_fn(&vblank->disable_timer); > - else if (!dev->vblank_disable_immediate) > + else if (__vblank_disable_immediate(dev, pipe)) > mod_timer(&vblank->disable_timer, > jiffies + ((drm_vblank_offdelay * HZ)/1000)); > } > @@ -1663,7 +1659,7 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data, > /* If the counter is currently enabled and accurate, short-circuit > * queries to return the cached timestamp of the last vblank. > */ > - if (dev->vblank_disable_immediate && > + if (__vblank_disable_immediate(dev, pipe) && > drm_wait_vblank_is_query(vblwait) && > READ_ONCE(vblank->enabled)) { > drm_wait_vblank_reply(dev, pipe, &vblwait->reply); > @@ -1820,7 +1816,7 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe) > * been signaled. The disable has to be last (after > * drm_handle_vblank_events) so that the timestamp is always accurate. > */ > - disable_irq = (dev->vblank_disable_immediate && > + disable_irq = (__vblank_disable_immediate(dev, pipe) && > drm_vblank_offdelay > 0 && > !atomic_read(&vblank->refcount)); > > @@ -1893,7 +1889,8 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data, > pipe = drm_crtc_index(crtc); > > vblank = &dev->vblank[pipe]; > - vblank_enabled = dev->vblank_disable_immediate && READ_ONCE(vblank->enabled); > + vblank_enabled = __vblank_disable_immediate(dev, pipe) && > + READ_ONCE(vblank->enabled); > > if (!vblank_enabled) { > ret = drm_crtc_vblank_get(crtc); > -- > 2.24.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi Am 15.01.20 um 15:37 schrieb Ville Syrjälä: > On Wed, Jan 15, 2020 at 01:16:33PM +0100, Thomas Zimmermann wrote: >> VBLANK interrupts can be disabled immediately or with a delay, where the >> latter is the default. The former option can be selected by setting >> get_vblank_timestamp, and enabling vblank_disable_immediate in struct >> drm_device. >> >> The setup is only evaluated once when DRM initializes VBLANKs. Evaluating >> the settings on each use of vblank_disable_immediate will allow for easy >> integration of CRTC VBLANK functions. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/gpu/drm/drm_vblank.c | 31 ++++++++++++++----------------- >> 1 file changed, 14 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c >> index 3f1dd54cc8bb..abb085c67d82 100644 >> --- a/drivers/gpu/drm/drm_vblank.c >> +++ b/drivers/gpu/drm/drm_vblank.c >> @@ -481,19 +481,6 @@ int drm_vblank_init(struct drm_device *dev, unsigned int num_crtcs) >> >> DRM_INFO("Supports vblank timestamp caching Rev 2 (21.10.2013).\n"); >> >> - /* Driver specific high-precision vblank timestamping supported? */ >> - if (dev->driver->get_vblank_timestamp) >> - DRM_INFO("Driver supports precise vblank timestamp query.\n"); >> - 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"); >> - } > > Which drivers are so broken they set vblank_disable_immediate to true > without having the vfunc specified? IMO this code should just go away > (or converted to a WARN). It's probably not a problem in practice. I'll put WARN_ON around the condition. Best regards Thomas > >> - >> return 0; >> >> err: >> @@ -1070,6 +1057,15 @@ int drm_crtc_vblank_get(struct drm_crtc *crtc) >> } >> EXPORT_SYMBOL(drm_crtc_vblank_get); >> >> +static bool __vblank_disable_immediate(struct drm_device *dev, unsigned int pipe) >> +{ >> + if (!dev->vblank_disable_immediate) >> + return false; >> + if (!dev->driver->get_vblank_timestamp) >> + return false; >> + return true; >> +} >> + >> static void drm_vblank_put(struct drm_device *dev, unsigned int pipe) >> { >> struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; >> @@ -1086,7 +1082,7 @@ static void drm_vblank_put(struct drm_device *dev, unsigned int pipe) >> return; >> else if (drm_vblank_offdelay < 0) >> vblank_disable_fn(&vblank->disable_timer); >> - else if (!dev->vblank_disable_immediate) >> + else if (__vblank_disable_immediate(dev, pipe)) >> mod_timer(&vblank->disable_timer, >> jiffies + ((drm_vblank_offdelay * HZ)/1000)); >> } >> @@ -1663,7 +1659,7 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data, >> /* If the counter is currently enabled and accurate, short-circuit >> * queries to return the cached timestamp of the last vblank. >> */ >> - if (dev->vblank_disable_immediate && >> + if (__vblank_disable_immediate(dev, pipe) && >> drm_wait_vblank_is_query(vblwait) && >> READ_ONCE(vblank->enabled)) { >> drm_wait_vblank_reply(dev, pipe, &vblwait->reply); >> @@ -1820,7 +1816,7 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe) >> * been signaled. The disable has to be last (after >> * drm_handle_vblank_events) so that the timestamp is always accurate. >> */ >> - disable_irq = (dev->vblank_disable_immediate && >> + disable_irq = (__vblank_disable_immediate(dev, pipe) && >> drm_vblank_offdelay > 0 && >> !atomic_read(&vblank->refcount)); >> >> @@ -1893,7 +1889,8 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data, >> pipe = drm_crtc_index(crtc); >> >> vblank = &dev->vblank[pipe]; >> - vblank_enabled = dev->vblank_disable_immediate && READ_ONCE(vblank->enabled); >> + vblank_enabled = __vblank_disable_immediate(dev, pipe) && >> + READ_ONCE(vblank->enabled); >> >> if (!vblank_enabled) { >> ret = drm_crtc_vblank_get(crtc); >> -- >> 2.24.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
VBLANK handlers in struct drm_driver are deprecated. Only legacy, non-KMS drivers are supposed to used them. DRM drivers with kernel modesetting are supposed to use VBLANK callbacks of the CRTC infrastructure. This patchset converts all DRM drivers to CRTC VBLANK callbacks and cleans up struct drm_driver. The remaining VBLANK callbacks in struct drm_driver are only used by legacy drivers. Patches 1 and 3 prepare the DRM infrastructure. These patches add get_scanout_position() to struct drm_crtc_helper_funcs, get_vblank_timestamp() to struct drm_crtc_funcs, and add helpers for the new interfaces. Patch 2 changes the VBLANK code to evaluate vblank_disable_immediate in struct drm_device. This simplifies the integration of CRTC VBLANK callbacks in patch 3. If necessary, a future patch could move vblank_disable_immedate to struct drm_crtc, so that high-precision VBLANKs could be enabled on a per-CRTC basis. Patches 4 to 20 convert drivers over. In patch 21, all VBLANK callbacks are removed from struct drm_driver, except for get_vblank_counter(), enable_vblank(), and disable_vblank(). These interfaces are moved to the legacy section at the end of the structure. Old helper code is now unused and being removed as well. To cover all affected drivers, I build the patchset in x86, x86-64, arm and aarch64. I smoke-tested amdgpu, gma500, i915, radeon and vc4 on respective hardware. v2: * reorder patches so the i915 can be converted without duplicating helper code. * merged cleanup patches * changed VBLANK function signatures in amdgpu Thomas Zimmermann (21): drm: Add get_scanout_position() to struct drm_crtc_helper_funcs drm: Evaluate struct drm_device.vblank_disable_immediate on each use drm: Add get_vblank_timestamp() to struct drm_crtc_funcs drm/amdgpu: Convert to struct drm_crtc_helper_funcs.get_scanout_position() drm/amdgpu: Convert to CRTC VBLANK callbacks drm/gma500: Convert to CRTC VBLANK callbacks drm/i915: Convert to CRTC VBLANK callbacks drm/nouveau: Convert to struct drm_crtc_helper_funcs.get_scanout_position() drm/nouveau: Convert to CRTC VBLANK callbacks drm/radeon: Convert to struct drm_crtc_helper_funcs.get_scanout_position() drm/radeon: Convert to CRTC VBLANK callbacks drm/msm: Convert to struct drm_crtc_helper_funcs.get_scanout_position() drm/msm: Convert to CRTC VBLANK callbacks drm/stm: Convert to struct drm_crtc_helper_funcs.get_scanout_position() drm/stm: Convert to CRTC VBLANK callbacks drm/sti: Convert to CRTC VBLANK callbacks drm/vc4: Convert to struct drm_crtc_helper_funcs.get_scanout_position() drm/vc4: Convert to CRTC VBLANK callbacks drm/vkms: Convert to CRTC VBLANK callbacks drm/vmwgfx: Convert to CRTC VBLANK callbacks drm: Clean-up VBLANK-related callbacks in struct drm_driver drivers/gpu/drm/amd/amdgpu/amdgpu.h | 6 +- drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 16 +- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 15 - drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 21 +- drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 5 + drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 5 + drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 5 + drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 5 + drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 5 + drivers/gpu/drm/amd/amdgpu/dce_virtual.c | 5 + .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 13 +- drivers/gpu/drm/drm_vblank.c | 411 ++++++++++-------- drivers/gpu/drm/gma500/cdv_intel_display.c | 3 + drivers/gpu/drm/gma500/psb_drv.c | 4 - drivers/gpu/drm/gma500/psb_drv.h | 6 +- drivers/gpu/drm/gma500/psb_intel_display.c | 3 + drivers/gpu/drm/gma500/psb_irq.c | 12 +- drivers/gpu/drm/gma500/psb_irq.h | 7 +- drivers/gpu/drm/i915/display/intel_display.c | 7 + drivers/gpu/drm/i915/i915_drv.c | 3 - drivers/gpu/drm/i915/i915_irq.c | 20 +- drivers/gpu/drm/i915/i915_irq.h | 6 +- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 + drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 2 + drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 82 ++++ drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 95 ---- drivers/gpu/drm/msm/msm_drv.c | 10 +- drivers/gpu/drm/msm/msm_drv.h | 3 + drivers/gpu/drm/nouveau/dispnv04/crtc.c | 4 + drivers/gpu/drm/nouveau/dispnv50/head.c | 5 + drivers/gpu/drm/nouveau/nouveau_display.c | 28 +- drivers/gpu/drm/nouveau/nouveau_display.h | 6 +- drivers/gpu/drm/nouveau/nouveau_drm.c | 5 - drivers/gpu/drm/radeon/atombios_crtc.c | 1 + drivers/gpu/drm/radeon/radeon_display.c | 25 +- drivers/gpu/drm/radeon/radeon_drv.c | 18 - drivers/gpu/drm/radeon/radeon_kms.c | 29 +- drivers/gpu/drm/radeon/radeon_legacy_crtc.c | 3 +- drivers/gpu/drm/radeon/radeon_mode.h | 6 + drivers/gpu/drm/sti/sti_crtc.c | 11 +- drivers/gpu/drm/sti/sti_crtc.h | 2 - drivers/gpu/drm/sti/sti_drv.c | 4 - drivers/gpu/drm/stm/drv.c | 2 - drivers/gpu/drm/stm/ltdc.c | 66 +-- drivers/gpu/drm/stm/ltdc.h | 5 - drivers/gpu/drm/vc4/vc4_crtc.c | 13 +- drivers/gpu/drm/vc4/vc4_drv.c | 3 - drivers/gpu/drm/vc4/vc4_drv.h | 4 - drivers/gpu/drm/vkms/vkms_crtc.c | 9 +- drivers/gpu/drm/vkms/vkms_drv.c | 1 - drivers/gpu/drm/vkms/vkms_drv.h | 4 - drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 3 - drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 6 +- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 6 +- drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 3 + drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 3 + drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 3 + include/drm/drm_crtc.h | 46 +- include/drm/drm_drv.h | 156 +------ include/drm/drm_modeset_helper_vtables.h | 47 ++ include/drm/drm_vblank.h | 30 +- 61 files changed, 692 insertions(+), 642 deletions(-) -- 2.24.1