From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH v3 2/4] drm: Initialize struct drm_crtc_state.no_vblank from device settings Date: Wed, 22 Jan 2020 09:47:56 +0100 Message-ID: <20200122084756.GQ43062@phenom.ffwll.local> References: <20200120122051.25178-1-tzimmermann@suse.de> <20200120122051.25178-3-tzimmermann@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20200120122051.25178-3-tzimmermann@suse.de> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: virtualization-bounces@lists.linux-foundation.org Sender: "Virtualization" To: Thomas Zimmermann Cc: david@lechnology.com, oleksandr_andrushchenko@epam.com, airlied@linux.ie, sam@ravnborg.org, dri-devel@lists.freedesktop.org, maarten.lankhorst@linux.intel.com, mripard@kernel.org, virtualization@lists.linux-foundation.org, hdegoede@redhat.com, noralf@tronnes.org, daniel@ffwll.ch, xen-devel@lists.xenproject.org, emil.velikov@collabora.com, sean@poorly.run, laurent.pinchart@ideasonboard.com List-Id: virtualization@lists.linuxfoundation.org On Mon, Jan 20, 2020 at 01:20:49PM +0100, Thomas Zimmermann wrote: > 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. > > First of all, the patch updates the documentation to make this behaviour > official: settings struct drm_crtc_state.no_vblank to true enables > automatic VBLANK generation. > > Atomic modesetting helper set the initial value of no_vblank in > drm_atomic_helper_check_modeset(). If vblanking has been initialized > for a CRTC, 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. > > As drivers previously send out VBLANK events by themselves, all > affected drivers have to be updated as well. Usually, deleting the > driver's vblanking code is sufficient. Xen implements its own logic > for generating events and therefore needs to override no_vblank > with a value of false. > > v3: > * squash all related changes patches into this patch Hm, since the fall-back only happens when the driver hasn't sent out the even I think it'd be safe to split the driver cleanups into a separate patch. Makes the core/helper changes stand out more properly. Even the xen hunk I think isn't strictly needed, since that pick up the event correctly and clears state->event to NULL. > > Signed-off-by: Thomas Zimmermann > --- > drivers/gpu/drm/arc/arcpgu_crtc.c | 16 -------------- > 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 | 9 ++++++++ > 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/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 ++++-- > 16 files changed, 56 insertions(+), 122 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 > #include > #include > -#include > #include > #include > #include > @@ -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, > }; > 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 > #include > #include > -#include > > #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 = { > 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 > #include > #include > -#include > > #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 = { > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 4511c2e07bb9..6e9c730a8919 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_crtc_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_crtc_has_vblank(crtc)) > + new_crtc_state->no_vblank = false; > + else > + new_crtc_state->no_vblank = true; Yeah this looks much better than my hack :-) > } > > 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. I still think we should reword this entire paragraph to make the "hw has no vblank" the main use-case, with writeback connectors as the "Also used for ..." special case. > * > * 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_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 > #include > #include > -#include > #include