Instead of faking VBLANK events by themselves, drivers without VBLANK support can enable drm_crtc_vblank.no_vblank and let DRM do the rest. The patchset makes this official and converts over drivers.
The current implementation looks at state of a device wrt vblanking. If vblanking has been initialized for the device, the driver is responsible for sending out VBLANK events. Otherwise, DRM will send out the event. The behaviour is selected by initializing no_vblank as part of drm_atomic_helper_check_modeset().
I went through all drivers, looking for those that call send out VBLANK events but do not call drm_vblank_init(). These are converted to the new semantics. This affects tiny drivers; drivers for virtual hardware; and a few others, which do not support interrupts. Xen comes with its own VBLANK logic and now disables no_vblank explictly.
v4: * replaced drm_crtc_has_vblank() with drm_dev_has_vblank() (Daniel) * squashed patches 1 and 2 * moved driver updates into separate patches v3: * reorder and squash patches * set no_vblank in drm_atomic_helper_check_modeset() for *all* drivers (Daniel) * convert all drivers to new semnatics as necessary v2: * document functionality (Daniel) * cleanup ast (Daniel) * let simple-kms handle no_vblank where possible
Thomas Zimmermann (15): drm: Initialize struct drm_crtc_state.no_vblank from device settings drm/arc: Remove sending of vblank event drm/ast: Don't set struct drm_crtc_state.no_vblank explictly drm/bochs: Remove sending of vblank event drm/cirrus: Remove sending of vblank event drm/gm12u320: Remove sending of vblank event drm/ili9225: Remove sending of vblank event drm/mipi-dbi: Remove sending of vblank event drm/qxl: Remove sending of vblank event drm/repaper: Remove sending of vblank event drm/st7586: Remove sending of vblank event drm/udl: Don't set struct drm_crtc_state.no_vblank explictly drm/vboxvideo: Remove sending of vblank event drm/virtio: Remove sending of vblank event drm/xen: Explicitly disable automatic sending of vblank event
drivers/gpu/drm/arc/arcpgu_crtc.c | 16 -------------- drivers/gpu/drm/ast/ast_mode.c | 2 -- drivers/gpu/drm/bochs/bochs_kms.c | 9 -------- drivers/gpu/drm/cirrus/cirrus.c | 8 ------- drivers/gpu/drm/drm_atomic_helper.c | 10 ++++++++- drivers/gpu/drm/drm_mipi_dbi.c | 9 -------- drivers/gpu/drm/drm_vblank.c | 28 ++++++++++++++++++++++++ drivers/gpu/drm/qxl/qxl_display.c | 14 ------------ drivers/gpu/drm/tiny/gm12u320.c | 9 -------- drivers/gpu/drm/tiny/ili9225.c | 9 -------- drivers/gpu/drm/tiny/repaper.c | 9 -------- drivers/gpu/drm/tiny/st7586.c | 9 -------- drivers/gpu/drm/udl/udl_modeset.c | 11 ---------- drivers/gpu/drm/vboxvideo/vbox_mode.c | 12 ---------- drivers/gpu/drm/virtio/virtgpu_display.c | 8 ------- drivers/gpu/drm/xen/xen_drm_front_kms.c | 13 +++++++++++ include/drm/drm_crtc.h | 27 +++++++++++++++++------ include/drm/drm_simple_kms_helper.h | 7 ++++-- include/drm/drm_vblank.h | 1 + 19 files changed, 76 insertions(+), 135 deletions(-)
-- 2.24.1
At the end of a commit, atomic helpers can generate a VBLANK event automatically. Originally implemented for writeback connectors, the functionality can be used by any driver and/or hardware without proper VBLANK interrupt.
The patch updates the documentation to make this behaviour official: settings struct drm_crtc_state.no_vblank to true enables automatic VBLANK generation.
The new interface drm_dev_has_vblank() returns true if vblanking has been initialized for a device, or false otherwise. This function will be useful when initializing no_vblank in the CRTC state.
Atomic modesetting helper set the initial value of no_vblank in drm_atomic_helper_check_modeset(). If vblanking has been initialized for a device, no_blank is disabled. Otherwise it's enabled. Hence, atomic helpers will automatically send out VBLANK events with any driver that did not initialize vblanking.
v4: * replace drm_crtc_has_vblank() with drm_dev_has_vblank() * add drm_dev_crtc_has_vblank() in this patch * move driver changes into separate patches v3: * squash all related changes patches into this patch
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Acked-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/drm_atomic_helper.c | 10 +++++++++- drivers/gpu/drm/drm_vblank.c | 28 ++++++++++++++++++++++++++++ include/drm/drm_crtc.h | 27 ++++++++++++++++++++------- include/drm/drm_simple_kms_helper.h | 7 +++++-- include/drm/drm_vblank.h | 1 + 5 files changed, 63 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 4511c2e07bb9..d7b73cd89b79 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -583,6 +583,7 @@ mode_valid(struct drm_atomic_state *state) * &drm_crtc_state.connectors_changed is set when a connector is added or * removed from the CRTC. &drm_crtc_state.active_changed is set when * &drm_crtc_state.active changes, which is used for DPMS. + * &drm_crtc_state.no_vblank is set from the result of drm_dev_has_vblank(). * See also: drm_atomic_crtc_needs_modeset() * * IMPORTANT: @@ -649,6 +650,11 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
return -EINVAL; } + + if (drm_dev_has_vblank(dev)) + new_crtc_state->no_vblank = false; + else + new_crtc_state->no_vblank = true; }
ret = handle_conflicting_encoders(state, false); @@ -2215,7 +2221,9 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies); * when a job is queued, and any change to the pipeline that does not touch the * connector is leading to timeouts when calling * drm_atomic_helper_wait_for_vblanks() or - * drm_atomic_helper_wait_for_flip_done(). + * drm_atomic_helper_wait_for_flip_done(). In addition to writeback + * connectors, this function can also fake VBLANK events for CRTCs without + * VBLANK interrupt. * * This is part of the atomic helper support for nonblocking commits, see * drm_atomic_helper_setup_commit() for an overview. diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 1659b13b178c..433dec6230b1 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -69,6 +69,12 @@ * &drm_driver.max_vblank_count. In that case the vblank core only disables the * vblanks after a timer has expired, which can be configured through the * ``vblankoffdelay`` module parameter. + * + * Drivers for hardware without support for vertical-blanking interrupts + * must not call drm_vblank_init(). For such drivers, atomic helpers will + * automatically generate vblank events as part of the display update. This + * functionality also can be controlled by the driver by enabling and disabling + * struct drm_crtc_state.no_vblank. */
/* Retry timestamp calculation up to 3 times to satisfy @@ -501,6 +507,28 @@ int drm_vblank_init(struct drm_device *dev, unsigned int num_crtcs) } EXPORT_SYMBOL(drm_vblank_init);
+/** + * drm_dev_has_vblank - test if vblanking has been initialized for + * a device + * @dev: the device + * + * Drivers may call this function to test if vblank support is + * initialized for a device. For most hardware this means that vblanking + * can also be enabled. + * + * Atomic helpers use this function to initialize + * &drm_crtc_state.no_vblank. See also drm_atomic_helper_check_modeset(). + * + * Returns: + * True if vblanking has been initialized for the given device, false + * otherwise. + */ +bool drm_dev_has_vblank(const struct drm_device *dev) +{ + return dev->num_crtcs != 0; +} +EXPORT_SYMBOL(drm_dev_has_vblank); + /** * drm_crtc_vblank_waitqueue - get vblank waitqueue for the CRTC * @crtc: which CRTC's vblank waitqueue to retrieve diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 5e9b15a0e8c5..5363e31c9abe 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -174,12 +174,22 @@ struct drm_crtc_state { * @no_vblank: * * Reflects the ability of a CRTC to send VBLANK events. This state - * usually depends on the pipeline configuration, and the main usuage - * is CRTCs feeding a writeback connector operating in oneshot mode. - * In this case the VBLANK event is only generated when a job is queued - * to the writeback connector, and we want the core to fake VBLANK - * events when this part of the pipeline hasn't changed but others had - * or when the CRTC and connectors are being disabled. + * usually depends on the pipeline configuration. If set to true, DRM + * atomic helpers will sendout a fake VBLANK event during display + * updates. + * + * One usage is for drivers and/or hardware without support for VBLANK + * interrupts. Such drivers typically do not initialize vblanking + * (i.e., call drm_vblank_init() wit the number of CRTCs). For CRTCs + * without initialized vblanking, the field is initialized to true and + * a VBLANK event will be send out on each update of the display + * pipeline. + * + * Another usage is CRTCs feeding a writeback connector operating in + * oneshot mode. In this case the VBLANK event is only generated when + * a job is queued to the writeback connector, and we want the core + * to fake VBLANK events when this part of the pipeline hasn't changed + * but others had or when the CRTC and connectors are being disabled. * * __drm_atomic_helper_crtc_duplicate_state() will not reset the value * from the current state, the CRTC driver is then responsible for @@ -335,7 +345,10 @@ struct drm_crtc_state { * - Events for disabled CRTCs are not allowed, and drivers can ignore * that case. * - * This can be handled by the drm_crtc_send_vblank_event() function, + * For very simple hardware without VBLANK interrupt, enabling + * &struct drm_crtc_state.no_vblank makes DRM's atomic commit helpers + * send the event at an appropriate time. For more complex hardware this + * can be handled by the drm_crtc_send_vblank_event() function, * which the driver should call on the provided event upon completion of * the atomic commit. Note that if the driver supports vblank signalling * and timestamping the vblank counters and timestamps must agree with diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h index 15afee9cf049..e253ba7bea9d 100644 --- a/include/drm/drm_simple_kms_helper.h +++ b/include/drm/drm_simple_kms_helper.h @@ -100,8 +100,11 @@ struct drm_simple_display_pipe_funcs { * This is the function drivers should submit the * &drm_pending_vblank_event from. Using either * drm_crtc_arm_vblank_event(), when the driver supports vblank - * interrupt handling, or drm_crtc_send_vblank_event() directly in case - * the hardware lacks vblank support entirely. + * interrupt handling, or drm_crtc_send_vblank_event() for more + * complex case. In case the hardware lacks vblank support entirely, + * drivers can set &struct drm_crtc_state.no_vblank in + * &struct drm_simple_display_pipe_funcs.check and let DRM's + * atomic helper fake a vblank event. */ void (*update)(struct drm_simple_display_pipe *pipe, struct drm_plane_state *old_plane_state); diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h index c16c44052b3d..94275e93fd27 100644 --- a/include/drm/drm_vblank.h +++ b/include/drm/drm_vblank.h @@ -206,6 +206,7 @@ struct drm_vblank_crtc { };
int drm_vblank_init(struct drm_device *dev, unsigned int num_crtcs); +bool drm_dev_has_vblank(const struct drm_device *dev); u64 drm_crtc_vblank_count(struct drm_crtc *crtc); u64 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc, ktime_t *vblanktime);
On Thu, Jan 23, 2020 at 10:21:09AM +0100, Thomas Zimmermann wrote:
drm_dev_has_vblank I guess?
s/sendout/ send out/
maby add "... updates, after all hardware changes have been committed. This is implemented in drm_atomic_helper_fake_vblank()."
That at least reflects the default behaviour.
s/wit/with/
"initialized to true" is a bit a stretch, since this is done in the helpers. Maybe instead "... this field is set to true in drm_atomic_helper_check_modeset() and ..."
Maybe also reiterate that this is done through drm_atomic_helper_fake_vblank() again? Imo for stuff done by helpers it's important to highligh where that's done, since it's all opt-in (even if most drivers will opt-in automatically).
With the doc nits addressed:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
-- 2.24.1
Hi Thomas,
On Thu, 23 Jan 2020 at 09:21, Thomas Zimmermann tzimmermann@suse.de wrote:
Perhaps it's just me, yet the following ideas would make the topic significantly easier and clearer.
- adding explicit "fake" when talking about drm/atomic _helpers_ generating/sending a VBLANK event. For example, in 15/15 the commit message says "fake", while inline comment omits it... Leading to the confusion pointed out.
- s/no_vblank/fake_vblank/g or s/no_vblank/no_hw_vblank/g Simple and concise. With slight inclination towards the former wording :-)
If you and Daniel agree with the rename, then the first sentence of the description should probably be tweaked.
HTH Emil
Hi Emil
Am 27.01.20 um 19:12 schrieb Emil Velikov:
No problem on being more precise here. I'll update the docs accordingly.
I'd prefer to not change the field's name. The current name 'no_vblank' indicates state and lets helpers decide what to do with it. The name 'fake_vblank' indicates an instruction to the helpers, telling them what to do. It does neither seem to fit into drm_crtc_state, nor into the overall concept.
Best regards Thomas
On Mon, Jan 27, 2020 at 07:42:27PM +0100, Thomas Zimmermann wrote:
Yeah e.g. xen has no hw vblank, but still has special processing of events, which are kinda triggered by the "hw" (it's an event from the compositor).
Maybe the confusion is with the helper function that generates the fake_vblank, since it's not really a fake vblank at all, it's just "send out this atomic completion event now, I'm not going to do it as part of the vblank processing since no vblank". So maybe that function should be called _send_events_i_have_no_hw_vblank, which yeah is not a great name :-) But maybe you have an idea for that one? -Daniel
The atomic helpers automatically send out fake VBLANK events if no vblanking has been initialized. Remove the sending code from the driver.
v4: * separate commit from core vblank changes
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Acked-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/arc/arcpgu_crtc.c | 16 ---------------- 1 file changed, 16 deletions(-)
diff --git a/drivers/gpu/drm/arc/arcpgu_crtc.c b/drivers/gpu/drm/arc/arcpgu_crtc.c index 8ae1e1f97a73..be7c29cec318 100644 --- a/drivers/gpu/drm/arc/arcpgu_crtc.c +++ b/drivers/gpu/drm/arc/arcpgu_crtc.c @@ -9,7 +9,6 @@ #include <drm/drm_device.h> #include <drm/drm_fb_cma_helper.h> #include <drm/drm_gem_cma_helper.h> -#include <drm/drm_vblank.h> #include <drm/drm_plane_helper.h> #include <drm/drm_probe_helper.h> #include <linux/clk.h> @@ -138,24 +137,9 @@ static void arc_pgu_crtc_atomic_disable(struct drm_crtc *crtc, ~ARCPGU_CTRL_ENABLE_MASK); }
-static void arc_pgu_crtc_atomic_begin(struct drm_crtc *crtc, - struct drm_crtc_state *state) -{ - struct drm_pending_vblank_event *event = crtc->state->event; - - if (event) { - crtc->state->event = NULL; - - spin_lock_irq(&crtc->dev->event_lock); - drm_crtc_send_vblank_event(crtc, event); - spin_unlock_irq(&crtc->dev->event_lock); - } -} - static const struct drm_crtc_helper_funcs arc_pgu_crtc_helper_funcs = { .mode_valid = arc_pgu_crtc_mode_valid, .mode_set_nofb = arc_pgu_crtc_mode_set_nofb, - .atomic_begin = arc_pgu_crtc_atomic_begin, .atomic_enable = arc_pgu_crtc_atomic_enable, .atomic_disable = arc_pgu_crtc_atomic_disable, };
As ast does not initialize vblanking, atomic helpers initialize the value of struct drm_crtc_state.no_vblank to be true. No need to set it from within the driver.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Acked-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/ast/ast_mode.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 34608f0499eb..7810a84e7e9e 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -833,8 +833,6 @@ static void ast_crtc_helper_atomic_flush(struct drm_crtc *crtc, struct ast_vbios_mode_info *vbios_mode_info; struct drm_display_mode *adjusted_mode;
- crtc->state->no_vblank = true; - ast_state = to_ast_crtc_state(crtc->state);
format = ast_state->format;
The atomic helpers automatically send out fake VBLANK events if no vblanking has been initialized. Remove the sending code from the driver.
v4: * separate commit from core vblank changes
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Acked-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/bochs/bochs_kms.c | 9 --------- 1 file changed, 9 deletions(-)
diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c index 3f0006c2470d..ff275faee88d 100644 --- a/drivers/gpu/drm/bochs/bochs_kms.c +++ b/drivers/gpu/drm/bochs/bochs_kms.c @@ -7,7 +7,6 @@ #include <drm/drm_atomic_helper.h> #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_probe_helper.h> -#include <drm/drm_vblank.h>
#include "bochs.h"
@@ -57,16 +56,8 @@ static void bochs_pipe_update(struct drm_simple_display_pipe *pipe, struct drm_plane_state *old_state) { struct bochs_device *bochs = pipe->crtc.dev->dev_private; - struct drm_crtc *crtc = &pipe->crtc;
bochs_plane_update(bochs, pipe->plane.state); - - if (crtc->state->event) { - spin_lock_irq(&crtc->dev->event_lock); - drm_crtc_send_vblank_event(crtc, crtc->state->event); - crtc->state->event = NULL; - spin_unlock_irq(&crtc->dev->event_lock); - } }
static const struct drm_simple_display_pipe_funcs bochs_pipe_funcs = {
The atomic helpers automatically send out fake VBLANK events if no vblanking has been initialized. Remove the sending code from the driver.
v4: * separate commit from core vblank changes
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Acked-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/cirrus/cirrus.c | 8 -------- 1 file changed, 8 deletions(-)
diff --git a/drivers/gpu/drm/cirrus/cirrus.c b/drivers/gpu/drm/cirrus/cirrus.c index 248c9f765c45..a91fb0d7282c 100644 --- a/drivers/gpu/drm/cirrus/cirrus.c +++ b/drivers/gpu/drm/cirrus/cirrus.c @@ -38,7 +38,6 @@ #include <drm/drm_modeset_helper_vtables.h> #include <drm/drm_probe_helper.h> #include <drm/drm_simple_kms_helper.h> -#include <drm/drm_vblank.h>
#define DRIVER_NAME "cirrus" #define DRIVER_DESC "qemu cirrus vga" @@ -434,13 +433,6 @@ static void cirrus_pipe_update(struct drm_simple_display_pipe *pipe,
if (drm_atomic_helper_damage_merged(old_state, state, &rect)) cirrus_fb_blit_rect(pipe->plane.state->fb, &rect); - - if (crtc->state->event) { - spin_lock_irq(&crtc->dev->event_lock); - drm_crtc_send_vblank_event(crtc, crtc->state->event); - crtc->state->event = NULL; - spin_unlock_irq(&crtc->dev->event_lock); - } }
static const struct drm_simple_display_pipe_funcs cirrus_pipe_funcs = {
The atomic helpers automatically send out fake VBLANK events if no vblanking has been initialized. Remove the sending code from the driver.
v4: * separate commit from core vblank changes
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Acked-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/tiny/gm12u320.c | 9 --------- 1 file changed, 9 deletions(-)
diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c index 94fb1f593564..a48173441ae0 100644 --- a/drivers/gpu/drm/tiny/gm12u320.c +++ b/drivers/gpu/drm/tiny/gm12u320.c @@ -22,7 +22,6 @@ #include <drm/drm_modeset_helper_vtables.h> #include <drm/drm_probe_helper.h> #include <drm/drm_simple_kms_helper.h> -#include <drm/drm_vblank.h>
static bool eco_mode; module_param(eco_mode, bool, 0644); @@ -610,18 +609,10 @@ static void gm12u320_pipe_update(struct drm_simple_display_pipe *pipe, struct drm_plane_state *old_state) { struct drm_plane_state *state = pipe->plane.state; - struct drm_crtc *crtc = &pipe->crtc; struct drm_rect rect;
if (drm_atomic_helper_damage_merged(old_state, state, &rect)) gm12u320_fb_mark_dirty(pipe->plane.state->fb, &rect); - - if (crtc->state->event) { - spin_lock_irq(&crtc->dev->event_lock); - drm_crtc_send_vblank_event(crtc, crtc->state->event); - crtc->state->event = NULL; - spin_unlock_irq(&crtc->dev->event_lock); - } }
static const struct drm_simple_display_pipe_funcs gm12u320_pipe_funcs = {
The atomic helpers automatically send out fake VBLANK events if no vblanking has been initialized. Remove the sending code from the driver.
v4: * separate commit from core vblank changes
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Acked-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/tiny/ili9225.c | 9 --------- 1 file changed, 9 deletions(-)
diff --git a/drivers/gpu/drm/tiny/ili9225.c b/drivers/gpu/drm/tiny/ili9225.c index c66acc566c2b..802fb8dde1b6 100644 --- a/drivers/gpu/drm/tiny/ili9225.c +++ b/drivers/gpu/drm/tiny/ili9225.c @@ -26,7 +26,6 @@ #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_mipi_dbi.h> #include <drm/drm_rect.h> -#include <drm/drm_vblank.h>
#define ILI9225_DRIVER_READ_CODE 0x00 #define ILI9225_DRIVER_OUTPUT_CONTROL 0x01 @@ -165,18 +164,10 @@ static void ili9225_pipe_update(struct drm_simple_display_pipe *pipe, struct drm_plane_state *old_state) { struct drm_plane_state *state = pipe->plane.state; - struct drm_crtc *crtc = &pipe->crtc; struct drm_rect rect;
if (drm_atomic_helper_damage_merged(old_state, state, &rect)) ili9225_fb_dirty(state->fb, &rect); - - if (crtc->state->event) { - spin_lock_irq(&crtc->dev->event_lock); - drm_crtc_send_vblank_event(crtc, crtc->state->event); - spin_unlock_irq(&crtc->dev->event_lock); - crtc->state->event = NULL; - } }
static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe,
The atomic helpers automatically send out fake VBLANK events if no vblanking has been initialized. Remove the sending code from the driver.
v4: * separate commit from core vblank changes
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Acked-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/drm_mipi_dbi.c | 9 --------- 1 file changed, 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c index 16bff1be4b8a..13b753cb3f67 100644 --- a/drivers/gpu/drm/drm_mipi_dbi.c +++ b/drivers/gpu/drm/drm_mipi_dbi.c @@ -24,7 +24,6 @@ #include <drm/drm_modes.h> #include <drm/drm_probe_helper.h> #include <drm/drm_rect.h> -#include <drm/drm_vblank.h> #include <video/mipi_display.h>
#define MIPI_DBI_MAX_SPI_READ_SPEED 2000000 /* 2MHz */ @@ -299,18 +298,10 @@ void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe, struct drm_plane_state *old_state) { struct drm_plane_state *state = pipe->plane.state; - struct drm_crtc *crtc = &pipe->crtc; struct drm_rect rect;
if (drm_atomic_helper_damage_merged(old_state, state, &rect)) mipi_dbi_fb_dirty(state->fb, &rect); - - if (crtc->state->event) { - spin_lock_irq(&crtc->dev->event_lock); - drm_crtc_send_vblank_event(crtc, crtc->state->event); - spin_unlock_irq(&crtc->dev->event_lock); - crtc->state->event = NULL; - } } EXPORT_SYMBOL(mipi_dbi_pipe_update);
The atomic helpers automatically send out fake VBLANK events if no vblanking has been initialized. Remove the sending code from the driver.
v4: * separate commit from core vblank changes
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Acked-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/qxl/qxl_display.c | 14 -------------- 1 file changed, 14 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index 16d73b22f3f5..ab4f8dd00400 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -31,7 +31,6 @@ #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_plane_helper.h> #include <drm/drm_probe_helper.h> -#include <drm/drm_vblank.h>
#include "qxl_drv.h" #include "qxl_object.h" @@ -372,19 +371,6 @@ static void qxl_crtc_update_monitors_config(struct drm_crtc *crtc, static void qxl_crtc_atomic_flush(struct drm_crtc *crtc, struct drm_crtc_state *old_crtc_state) { - struct drm_device *dev = crtc->dev; - struct drm_pending_vblank_event *event; - unsigned long flags; - - if (crtc->state && crtc->state->event) { - event = crtc->state->event; - crtc->state->event = NULL; - - spin_lock_irqsave(&dev->event_lock, flags); - drm_crtc_send_vblank_event(crtc, event); - spin_unlock_irqrestore(&dev->event_lock, flags); - } - qxl_crtc_update_monitors_config(crtc, "flush"); }
The atomic helpers automatically send out fake VBLANK events if no vblanking has been initialized. Remove the sending code from the driver.
v4: * separate commit from core vblank changes
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Acked-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/tiny/repaper.c | 9 --------- 1 file changed, 9 deletions(-)
diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c index 76d179200775..183484595aea 100644 --- a/drivers/gpu/drm/tiny/repaper.c +++ b/drivers/gpu/drm/tiny/repaper.c @@ -33,7 +33,6 @@ #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_modes.h> #include <drm/drm_rect.h> -#include <drm/drm_vblank.h> #include <drm/drm_probe_helper.h> #include <drm/drm_simple_kms_helper.h>
@@ -856,18 +855,10 @@ static void repaper_pipe_update(struct drm_simple_display_pipe *pipe, struct drm_plane_state *old_state) { struct drm_plane_state *state = pipe->plane.state; - struct drm_crtc *crtc = &pipe->crtc; struct drm_rect rect;
if (drm_atomic_helper_damage_merged(old_state, state, &rect)) repaper_fb_dirty(state->fb); - - if (crtc->state->event) { - spin_lock_irq(&crtc->dev->event_lock); - drm_crtc_send_vblank_event(crtc, crtc->state->event); - spin_unlock_irq(&crtc->dev->event_lock); - crtc->state->event = NULL; - } }
static const struct drm_simple_display_pipe_funcs repaper_pipe_funcs = {
The atomic helpers automatically send out fake VBLANK events if no vblanking has been initialized. Remove the sending code from the driver.
v4: * separate commit from core vblank changes
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Acked-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/tiny/st7586.c | 9 --------- 1 file changed, 9 deletions(-)
diff --git a/drivers/gpu/drm/tiny/st7586.c b/drivers/gpu/drm/tiny/st7586.c index 060cc756194f..9ef559dd3191 100644 --- a/drivers/gpu/drm/tiny/st7586.c +++ b/drivers/gpu/drm/tiny/st7586.c @@ -23,7 +23,6 @@ #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_mipi_dbi.h> #include <drm/drm_rect.h> -#include <drm/drm_vblank.h>
/* controller-specific commands */ #define ST7586_DISP_MODE_GRAY 0x38 @@ -159,18 +158,10 @@ static void st7586_pipe_update(struct drm_simple_display_pipe *pipe, struct drm_plane_state *old_state) { struct drm_plane_state *state = pipe->plane.state; - struct drm_crtc *crtc = &pipe->crtc; struct drm_rect rect;
if (drm_atomic_helper_damage_merged(old_state, state, &rect)) st7586_fb_dirty(state->fb, &rect); - - if (crtc->state->event) { - spin_lock_irq(&crtc->dev->event_lock); - drm_crtc_send_vblank_event(crtc, crtc->state->event); - spin_unlock_irq(&crtc->dev->event_lock); - crtc->state->event = NULL; - } }
static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
As udl does not initialize vblanking, atomic helpers initialize the value of struct drm_crtc_state.no_vblank to be true. No need to set it from within the driver.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Acked-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/udl/udl_modeset.c | 11 ----------- 1 file changed, 11 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c index 22af17959053..d59ebac70b15 100644 --- a/drivers/gpu/drm/udl/udl_modeset.c +++ b/drivers/gpu/drm/udl/udl_modeset.c @@ -375,8 +375,6 @@ udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe, char *wrptr; int color_depth = UDL_COLOR_DEPTH_16BPP;
- crtc_state->no_vblank = true; - buf = (char *)udl->mode_buf;
/* This first section has to do with setting the base address on the @@ -428,14 +426,6 @@ udl_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe) udl_submit_urb(dev, urb, buf - (char *)urb->transfer_buffer); }
-static int -udl_simple_display_pipe_check(struct drm_simple_display_pipe *pipe, - struct drm_plane_state *plane_state, - struct drm_crtc_state *crtc_state) -{ - return 0; -} - static void udl_simple_display_pipe_update(struct drm_simple_display_pipe *pipe, struct drm_plane_state *old_plane_state) @@ -457,7 +447,6 @@ struct drm_simple_display_pipe_funcs udl_simple_display_pipe_funcs = { .mode_valid = udl_simple_display_pipe_mode_valid, .enable = udl_simple_display_pipe_enable, .disable = udl_simple_display_pipe_disable, - .check = udl_simple_display_pipe_check, .update = udl_simple_display_pipe_update, .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb, };
The atomic helpers automatically send out fake VBLANK events if no vblanking has been initialized. Remove the sending code from the driver.
v4: * separate commit from core vblank changes
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Acked-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/vboxvideo/vbox_mode.c | 12 ------------ 1 file changed, 12 deletions(-)
diff --git a/drivers/gpu/drm/vboxvideo/vbox_mode.c b/drivers/gpu/drm/vboxvideo/vbox_mode.c index 19612132c8a3..8b7f005c4d20 100644 --- a/drivers/gpu/drm/vboxvideo/vbox_mode.c +++ b/drivers/gpu/drm/vboxvideo/vbox_mode.c @@ -18,7 +18,6 @@ #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_plane_helper.h> #include <drm/drm_probe_helper.h> -#include <drm/drm_vblank.h>
#include "hgsmi_channels.h" #include "vbox_drv.h" @@ -226,17 +225,6 @@ static void vbox_crtc_atomic_disable(struct drm_crtc *crtc, static void vbox_crtc_atomic_flush(struct drm_crtc *crtc, struct drm_crtc_state *old_crtc_state) { - struct drm_pending_vblank_event *event; - unsigned long flags; - - if (crtc->state && crtc->state->event) { - event = crtc->state->event; - crtc->state->event = NULL; - - spin_lock_irqsave(&crtc->dev->event_lock, flags); - drm_crtc_send_vblank_event(crtc, event); - spin_unlock_irqrestore(&crtc->dev->event_lock, flags); - } }
static const struct drm_crtc_helper_funcs vbox_crtc_helper_funcs = {
The atomic helpers automatically send out fake VBLANK events if no vblanking has been initialized. Remove the sending code from the driver.
v4: * separate commit from core vblank changes
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Acked-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/virtio/virtgpu_display.c | 8 -------- 1 file changed, 8 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c index 0966208ec30d..ecf4ba7cc32b 100644 --- a/drivers/gpu/drm/virtio/virtgpu_display.c +++ b/drivers/gpu/drm/virtio/virtgpu_display.c @@ -30,7 +30,6 @@ #include <drm/drm_fourcc.h> #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_probe_helper.h> -#include <drm/drm_vblank.h>
#include "virtgpu_drv.h"
@@ -121,13 +120,6 @@ static int virtio_gpu_crtc_atomic_check(struct drm_crtc *crtc, static void virtio_gpu_crtc_atomic_flush(struct drm_crtc *crtc, struct drm_crtc_state *old_state) { - unsigned long flags; - - spin_lock_irqsave(&crtc->dev->event_lock, flags); - if (crtc->state->event) - drm_crtc_send_vblank_event(crtc, crtc->state->event); - crtc->state->event = NULL; - spin_unlock_irqrestore(&crtc->dev->event_lock, flags); }
static const struct drm_crtc_helper_funcs virtio_gpu_crtc_helper_funcs = {
The atomic helpers automatically send out fake VBLANK events if no vblanking has been initialized. This would apply to xen, but xen has its own vblank logic. To avoid interfering with the atomic helpers, disable automatic vblank events explictly.
v4: * separate commit from core vblank changes
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Acked-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/xen/xen_drm_front_kms.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/xen/xen_drm_front_kms.c b/drivers/gpu/drm/xen/xen_drm_front_kms.c index 4f34c5208180..efde4561836f 100644 --- a/drivers/gpu/drm/xen/xen_drm_front_kms.c +++ b/drivers/gpu/drm/xen/xen_drm_front_kms.c @@ -220,6 +220,18 @@ static bool display_send_page_flip(struct drm_simple_display_pipe *pipe, return false; }
+static int display_check(struct drm_simple_display_pipe *pipe, + struct drm_plane_state *plane_state, + struct drm_crtc_state *crtc_state) +{ + /* Make sure that DRM helpers don't send VBLANK events + * automatically. Xen has it's own logic to do so. + */ + crtc_state->no_vblank = false; + + return 0; +} + static void display_update(struct drm_simple_display_pipe *pipe, struct drm_plane_state *old_plane_state) { @@ -284,6 +296,7 @@ static const struct drm_simple_display_pipe_funcs display_funcs = { .enable = display_enable, .disable = display_disable, .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb, + .check = display_check, .update = display_update, };
On Thu, Jan 23, 2020 at 10:21:23AM +0100, Thomas Zimmermann wrote:
On all the driver patches:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
So this here is strictly speaking dead code, because the fake_vblank helper makes sure to only send out the event if it hasn't been processed yet.
Which is a bit awkward, since it does this silently, potentially hiding bugs. We already have a warning that complains if a crtc_state->event hasn't been processed at all. But with the auto-vblank stuff here we kinda defeat that a bit ... I think adding a WARN_ON in fake_vblank that fires if no_vblank is set, but the event is somehow gone, would be really useful. That would point at some serious driver snafu ...
Care to throw that on top? -Daniel
Sorry for jumping in late
On 1/23/20 11:21 AM, Thomas Zimmermann wrote:
Reviewed-by: Oleksandr Andrushchenko oleksandr_andrushchenko@epam.com
Could you please put the comment on a separate line?
And it is still confusing, e.g. comment says "Make sure that DRM helpers don't send VBLANK" and we set "no_vblank" flag to false...
Hi
Am 27.01.20 um 10:53 schrieb Oleksandr Andrushchenko:
You mean to add an empty line between comment and code?
I'll rephrase and add some more context.
Best regards Thomas
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Emil Velikov -
Oleksandr Andrushchenko -
Thomas Zimmermann