[PATCH v2 0/4] Use no_vblank property for drivers without VBLANK

[PATCH v2 0/4] Use no_vblank property for drivers without VBLANK

[Thomas Zimmermann]

(Resending because I did not cc dri-devel properly.)

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 several drivers.

Ast already uses the functionality and just needs a cleanup. Cirrus can
be converted easily by setting the field in the check() callback and
removing the existing VBLANK code. For most other simple-KMS drivers
without enable_vblank() and check(), simple-KMS helpers can enable the
faked VBLANK by default. The only exception is Xen, which comes with
its own VBLANK logic and should rather to disable no_vblank.

v2:
	* document functionality (Daniel)
	* cleanup ast (Daniel)
	* let simple-kms handle no_vblank where possible

Thomas Zimmermann (4):
  drm: Document struct drm_crtc_state.no_vblank for faking VBLANK events
  drm/ast: Set struct drm_crtc_state.no_vblank in atomic_check()
  drm/cirrus: Let DRM core send VBLANK events
  drm/simple-kms: Let DRM core send VBLANK events by default

 drivers/gpu/drm/ast/ast_mode.c          |  4 ++--
 drivers/gpu/drm/bochs/bochs_kms.c       |  9 ---------
 drivers/gpu/drm/cirrus/cirrus.c         | 10 ++--------
 drivers/gpu/drm/drm_atomic_helper.c     |  4 +++-
 drivers/gpu/drm/drm_mipi_dbi.c          |  9 ---------
 drivers/gpu/drm/drm_simple_kms_helper.c | 19 +++++++++++++++----
 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/xen/xen_drm_front_kms.c | 13 +++++++++++++
 include/drm/drm_crtc.h                  |  9 +++++++--
 include/drm/drm_simple_kms_helper.h     |  7 +++++--
 14 files changed, 47 insertions(+), 84 deletions(-)

--
2.24.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/4] drm: Document struct drm_crtc_state.no_vblank for faking VBLANK events

[Thomas Zimmermann]

Drivers for CRTC hardware without support for VBLANK interrupts can set
struct drm_crtc_state.no_vblank and let DRM's atomic commit helpers
generate the VBLANK events automatically. Document this in order to make
it official.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_atomic_helper.c | 4 +++-
 include/drm/drm_crtc.h              | 9 +++++++--
 include/drm/drm_simple_kms_helper.h | 7 +++++--
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 4511c2e07bb9..ce30a37971e4 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2215,7 +2215,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/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 5e9b15a0e8c5..01caf5160596 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -179,7 +179,9 @@ struct drm_crtc_state {
 	 * 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.
+	 * or when the CRTC and connectors are being disabled. In addition to
+	 * writeback connectors, this function can also fake VBLANK events for
+	 * CRTCs without VBLANK interrupt.
 	 *
 	 * __drm_atomic_helper_crtc_duplicate_state() will not reset the value
 	 * from the current state, the CRTC driver is then responsible for
@@ -335,7 +337,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);
-- 
2.24.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/4] drm/ast: Set struct drm_crtc_state.no_vblank in atomic_check()

[Thomas Zimmermann]

CRTC state properties should be computed in atomic_check(). Do so for
the no_vblank field.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_mode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 34608f0499eb..ef7a0b08cc05 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -800,6 +800,8 @@ static int ast_crtc_helper_atomic_check(struct drm_crtc *crtc,
 		return -EINVAL;
 	}
 
+	state->no_vblank = true;
+
 	ast_state = to_ast_crtc_state(state);
 
 	format = ast_state->format;
@@ -833,8 +835,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;
-- 
2.24.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/4] drm/cirrus: Let DRM core send VBLANK events

[Thomas Zimmermann]

In drm_atomic_helper_fake_vblank(), the DRM core sends out VBLANK
events if struct drm_crtc_state.no_vblank is enabled. Replace cirrus'
VBLANK events with the DRM core's functionality.

v2:
	* set struct_drm_crtc_state.no_vblank in cirrus_pipe_check()

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/cirrus/cirrus.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/cirrus/cirrus.c b/drivers/gpu/drm/cirrus/cirrus.c
index 248c9f765c45..5ff15e8a2a0a 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"
@@ -404,6 +403,8 @@ static int cirrus_pipe_check(struct drm_simple_display_pipe *pipe,
 {
 	struct drm_framebuffer *fb = plane_state->fb;
 
+	crtc_state->no_vblank = true;
+
 	if (!fb)
 		return 0;
 	return cirrus_check_size(fb->width, fb->height, fb);
@@ -434,13 +435,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 = {
-- 
2.24.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 4/4] drm/simple-kms: Let DRM core send VBLANK events by default

[Thomas Zimmermann]

In drm_atomic_helper_fake_vblank() the DRM core sends out VBLANK events
if struct drm_crtc_state.no_vblank is enabled in the check() callbacks.

For drivers that have neither an enable_vblank() callback nor a check()
callback, the simple-KMS helpers enable VBLANK generation by default. This
simplifies bochs, udl, several tiny drivers, and drivers based upon MIPI
DPI helpers. The driver for Xen explicitly disables no_vblank, as it has
its own logic for sending these events.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/bochs/bochs_kms.c       |  9 ---------
 drivers/gpu/drm/drm_mipi_dbi.c          |  9 ---------
 drivers/gpu/drm/drm_simple_kms_helper.c | 19 +++++++++++++++----
 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/xen/xen_drm_front_kms.c | 13 +++++++++++++
 9 files changed, 28 insertions(+), 69 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 = {
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);
 
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
index 15fb516ae2d8..4414c7a5b2ce 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -146,10 +146,21 @@ static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
 	if (!plane_state->visible)
 		return 0;
 
-	if (!pipe->funcs || !pipe->funcs->check)
-		return 0;
-
-	return pipe->funcs->check(pipe, plane_state, crtc_state);
+	if (pipe->funcs) {
+		if (pipe->funcs->check)
+			return pipe->funcs->check(pipe, plane_state,
+						  crtc_state);
+		if (pipe->funcs->enable_vblank)
+			return 0;
+	}
+
+	/* Drivers without VBLANK support have to fake VBLANK events. As
+	 * there's no check() callback to enable this, set the no_vblank
+	 * field by default.
+	 */
+	crtc_state->no_vblank = true;
+
+	return 0;
 }
 
 static void drm_simple_kms_plane_atomic_update(struct drm_plane *plane,
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 = {
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,
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 = {
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,
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,
 };
diff --git a/drivers/gpu/drm/xen/xen_drm_front_kms.c b/drivers/gpu/drm/xen/xen_drm_front_kms.c
index 4f34c5208180..8ec29d5d3353 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 the simple DRM helpers don't enable VBLANK
+	 * generation. 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,
 };
 
-- 
2.24.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/4] drm/simple-kms: Let DRM core send VBLANK events by default

[Daniel Vetter]

On Wed, Jan 15, 2020 at 01:52:26PM +0100, Thomas Zimmermann wrote:
> In drm_atomic_helper_fake_vblank() the DRM core sends out VBLANK events
> if struct drm_crtc_state.no_vblank is enabled in the check() callbacks.
> 
> For drivers that have neither an enable_vblank() callback nor a check()
> callback, the simple-KMS helpers enable VBLANK generation by default. This
> simplifies bochs, udl, several tiny drivers, and drivers based upon MIPI
> DPI helpers. The driver for Xen explicitly disables no_vblank, as it has
> its own logic for sending these events.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> index 15fb516ae2d8..4414c7a5b2ce 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -146,10 +146,21 @@ static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
>  	if (!plane_state->visible)
>  		return 0;
>  
> -	if (!pipe->funcs || !pipe->funcs->check)
> -		return 0;
> -
> -	return pipe->funcs->check(pipe, plane_state, crtc_state);
> +	if (pipe->funcs) {
> +		if (pipe->funcs->check)
> +			return pipe->funcs->check(pipe, plane_state,
> +						  crtc_state);
> +		if (pipe->funcs->enable_vblank)
> +			return 0;
> +	}
> +
> +	/* Drivers without VBLANK support have to fake VBLANK events. As
> +	 * there's no check() callback to enable this, set the no_vblank
> +	 * field by default.
> +	 */

The ->check callback is right above this comment ... I'm confused.

> +	crtc_state->no_vblank = true;

That's kinda not what I meant with handling this automatically. Instead
something like this:


diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index 7cf3cf936547..23d2f51fc1d4 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -149,6 +149,11 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
 	/* Self refresh should be canceled when a new update is available */
 	state->active = drm_atomic_crtc_effectively_active(state);
 	state->self_refresh_active = false;
+
+	if (drm_dev_has_vblank(crtc->dev))
+		state->no_vblank = true;
+	else
+		state->no_vblank = false;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_crtc_duplicate_state);
 
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 1659b13b178c..32cab3d3c872 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -81,6 +81,12 @@
  */
 #define DRM_REDUNDANT_VBLIRQ_THRESH_NS 1000000
 
+/* FIXME roll this out here in this file */
+bool drm_dev_has_vblank(dev)
+{
+	return dev->num_crtcs;
+}
+
 static bool
 drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
 			  ktime_t *tvblank, bool in_vblank_irq);


But maybe move the default value to some other/better place in the atomic
helpers, not sure what the best one is.

Plus then in the documentation patch also highlight the link between
crtc_state->no_vblank and drm_dev_has_vblank respectively
drm_device.num_crtcs.

That should plug this issue once for all across the board.

There's still the fun between having the vblank callbacks and the
drm_vblank setup, but that's a much older can of worms ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/4] drm/simple-kms: Let DRM core send VBLANK events by default

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 4523 bytes --]

Hi

Am 16.01.20 um 07:41 schrieb Daniel Vetter:
> On Wed, Jan 15, 2020 at 01:52:26PM +0100, Thomas Zimmermann wrote:
>> In drm_atomic_helper_fake_vblank() the DRM core sends out VBLANK events
>> if struct drm_crtc_state.no_vblank is enabled in the check() callbacks.
>>
>> For drivers that have neither an enable_vblank() callback nor a check()
>> callback, the simple-KMS helpers enable VBLANK generation by default. This
>> simplifies bochs, udl, several tiny drivers, and drivers based upon MIPI
>> DPI helpers. The driver for Xen explicitly disables no_vblank, as it has
>> its own logic for sending these events.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
>> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
>> index 15fb516ae2d8..4414c7a5b2ce 100644
>> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
>> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
>> @@ -146,10 +146,21 @@ static int drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
>>  	if (!plane_state->visible)
>>  		return 0;
>>  
>> -	if (!pipe->funcs || !pipe->funcs->check)
>> -		return 0;
>> -
>> -	return pipe->funcs->check(pipe, plane_state, crtc_state);
>> +	if (pipe->funcs) {
>> +		if (pipe->funcs->check)
>> +			return pipe->funcs->check(pipe, plane_state,
>> +						  crtc_state);
>> +		if (pipe->funcs->enable_vblank)
>> +			return 0;
>> +	}
>> +
>> +	/* Drivers without VBLANK support have to fake VBLANK events. As
>> +	 * there's no check() callback to enable this, set the no_vblank
>> +	 * field by default.
>> +	 */
> 
> The ->check callback is right above this comment ... I'm confused.

I guess that comment isn't overly precise. What it means is that
no_vblank would have to be set in check(), but the driver did not
specify a check() function. So it has neither vblank support nor any way
of setting no_vblank. Hence, the simple-kms helper sets no_vblank
automatically.

Maybe something to update for the patchset's v2.

> 
>> +	crtc_state->no_vblank = true;
> 
> That's kinda not what I meant with handling this automatically. Instead
> something like this:
> 
> 
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index 7cf3cf936547..23d2f51fc1d4 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -149,6 +149,11 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
>  	/* Self refresh should be canceled when a new update is available */
>  	state->active = drm_atomic_crtc_effectively_active(state);
>  	state->self_refresh_active = false;
> +
> +	if (drm_dev_has_vblank(crtc->dev))
> +		state->no_vblank = true;
> +	else
> +		state->no_vblank = false;
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_crtc_duplicate_state);

I think the if/else branches are in the wrong order.

But generally speaking, is it really that easy? The xen driver already
has to work around simple-kms's auto-enabling of no_vblank (see patch
4). Maybe this settings interferes with other drivers as well. At least
the calls for sending fake vblanks should be removed from all affected
drivers.

Best regards
Thomas

>  
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 1659b13b178c..32cab3d3c872 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -81,6 +81,12 @@
>   */
>  #define DRM_REDUNDANT_VBLIRQ_THRESH_NS 1000000
>  
> +/* FIXME roll this out here in this file */
> +bool drm_dev_has_vblank(dev)
> +{
> +	return dev->num_crtcs;
> +}
> +
>  static bool
>  drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
>  			  ktime_t *tvblank, bool in_vblank_irq);
> 
> 
> But maybe move the default value to some other/better place in the atomic
> helpers, not sure what the best one is.
> 
> Plus then in the documentation patch also highlight the link between
> crtc_state->no_vblank and drm_dev_has_vblank respectively
> drm_device.num_crtcs.
> 
> That should plug this issue once for all across the board.
> 
> There's still the fun between having the vblank callbacks and the
> drm_vblank setup, but that's a much older can of worms ...
> -Daniel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/4] drm/simple-kms: Let DRM core send VBLANK events by default

[Emil Velikov]

Hi all,

On Thu, 16 Jan 2020 at 07:37, Thomas Zimmermann <tzimmermann@suse.de> wrote:

> > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> > index 7cf3cf936547..23d2f51fc1d4 100644
> > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > @@ -149,6 +149,11 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
> >       /* Self refresh should be canceled when a new update is available */
> >       state->active = drm_atomic_crtc_effectively_active(state);
> >       state->self_refresh_active = false;
> > +
> > +     if (drm_dev_has_vblank(crtc->dev))
> > +             state->no_vblank = true;
> > +     else
> > +             state->no_vblank = false;
> >  }
> >  EXPORT_SYMBOL(__drm_atomic_helper_crtc_duplicate_state);
>
> I think the if/else branches are in the wrong order.
>
> But generally speaking, is it really that easy? The xen driver already
> has to work around simple-kms's auto-enabling of no_vblank (see patch
> 4). Maybe this settings interferes with other drivers as well. At least
> the calls for sending fake vblanks should be removed from all affected
> drivers.
>

I'm not sure if setting no_vblank based on dev->num_crtcs is the correct thing.
From the original commit and associated description for no_vblank:

In some cases CRTCs are active but are not able to generating events, at
least not at every frame at it's expected to.
This is typically the case when the CRTC is feeding a writeback connector...

Reflects the ability of a CRTC to send VBLANK events....


The proposed handling of no_vblank feels a little dirty, although
nothing better comes to mind.
Nevertheless code seems perfectly reasonable, so if it were me I'd merge it.

HTH
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/4] drm/simple-kms: Let DRM core send VBLANK events by default

[Daniel Vetter]

On Thu, Jan 16, 2020 at 05:22:34PM +0000, Emil Velikov wrote:
> Hi all,
> 
> On Thu, 16 Jan 2020 at 07:37, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> 
> > > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> > > index 7cf3cf936547..23d2f51fc1d4 100644
> > > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > > @@ -149,6 +149,11 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
> > >       /* Self refresh should be canceled when a new update is available */
> > >       state->active = drm_atomic_crtc_effectively_active(state);
> > >       state->self_refresh_active = false;
> > > +
> > > +     if (drm_dev_has_vblank(crtc->dev))
> > > +             state->no_vblank = true;
> > > +     else
> > > +             state->no_vblank = false;
> > >  }
> > >  EXPORT_SYMBOL(__drm_atomic_helper_crtc_duplicate_state);
> >
> > I think the if/else branches are in the wrong order.

Yeah fumbled that.

> > But generally speaking, is it really that easy? The xen driver already
> > has to work around simple-kms's auto-enabling of no_vblank (see patch
> > 4). Maybe this settings interferes with other drivers as well. At least
> > the calls for sending fake vblanks should be removed from all affected
> > drivers.

Hm xen is really special, in that it has a flip complete event, but not a
vblank. I think forcing drivers to overwrite stuff in that case makes
sense.

> I'm not sure if setting no_vblank based on dev->num_crtcs is the correct thing.
> From the original commit and associated description for no_vblank:
> 
> In some cases CRTCs are active but are not able to generating events, at
> least not at every frame at it's expected to.
> This is typically the case when the CRTC is feeding a writeback connector...

Yeah, but Thomas' series here wants to extend that. And I think if we roll
this out the common case will be "no hw vblank", and the writeback special
case is going to be the exception to the exception. Yup, patch 1 that
updates the docs doesn't reflect that, which is why I'm bringing up more
suggestions here around code & semantics of all these pieces to make them
do the most reasonable thing for most of the drivers.

> Reflects the ability of a CRTC to send VBLANK events....
> 
> 
> The proposed handling of no_vblank feels a little dirty, although
> nothing better comes to mind.
> Nevertheless code seems perfectly reasonable, so if it were me I'd merge it.

The idea with setting it very early is that drivers can overwrite it very
easily. Feels slightly dirty, so I guess we could also set it somewhere in
the atomic_helper_check function (similar to how we set the various
crtc->*_changed flags, but we're not entirely consistent on these either).

For the overall thing what feels irky to me is making this no_vblank
default logic (however we end up computing it in the end, whether like
this or what I suggested) specific to simple pipe helpers feels kinda
wrong. Simple pipe tends to have a higher ratio of drivers for hw without
vblank support, but by far not the only ones. Having that special case
feels confusing to me (and likely will trip up some people, vblank and
event handling is already a huge source of confusion in drm).

One idea behind drm_dev_has_vblank() is also that we could formalize a bit
all that, at least for the usual case - xen and maybe others being some
exceptions as usual (hence definitely not something the core code should
handle).

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/4] drm/simple-kms: Let DRM core send VBLANK events by default

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 4411 bytes --]

Hi

Am 17.01.20 um 00:59 schrieb Daniel Vetter:
> On Thu, Jan 16, 2020 at 05:22:34PM +0000, Emil Velikov wrote:
>> Hi all,
>>
>> On Thu, 16 Jan 2020 at 07:37, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
>>>> index 7cf3cf936547..23d2f51fc1d4 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
>>>> @@ -149,6 +149,11 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
>>>>       /* Self refresh should be canceled when a new update is available */
>>>>       state->active = drm_atomic_crtc_effectively_active(state);
>>>>       state->self_refresh_active = false;
>>>> +
>>>> +     if (drm_dev_has_vblank(crtc->dev))
>>>> +             state->no_vblank = true;
>>>> +     else
>>>> +             state->no_vblank = false;
>>>>  }
>>>>  EXPORT_SYMBOL(__drm_atomic_helper_crtc_duplicate_state);
>>>
>>> I think the if/else branches are in the wrong order.
> 
> Yeah fumbled that.
> 
>>> But generally speaking, is it really that easy? The xen driver already
>>> has to work around simple-kms's auto-enabling of no_vblank (see patch
>>> 4). Maybe this settings interferes with other drivers as well. At least
>>> the calls for sending fake vblanks should be removed from all affected
>>> drivers.
> 
> Hm xen is really special, in that it has a flip complete event, but not a
> vblank. I think forcing drivers to overwrite stuff in that case makes
> sense.
> 
>> I'm not sure if setting no_vblank based on dev->num_crtcs is the correct thing.
>> From the original commit and associated description for no_vblank:
>>
>> In some cases CRTCs are active but are not able to generating events, at
>> least not at every frame at it's expected to.
>> This is typically the case when the CRTC is feeding a writeback connector...
> 
> Yeah, but Thomas' series here wants to extend that. And I think if we roll
> this out the common case will be "no hw vblank", and the writeback special

Default values should usually be 0 for zalloc and static initializers.
Should we rename no_vblank to has_vblank then?

> case is going to be the exception to the exception. Yup, patch 1 that
> updates the docs doesn't reflect that, which is why I'm bringing up more
> suggestions here around code & semantics of all these pieces to make them
> do the most reasonable thing for most of the drivers.
> 
>> Reflects the ability of a CRTC to send VBLANK events....
>>
>>
>> The proposed handling of no_vblank feels a little dirty, although
>> nothing better comes to mind.
>> Nevertheless code seems perfectly reasonable, so if it were me I'd merge it.
> 
> The idea with setting it very early is that drivers can overwrite it very
> easily. Feels slightly dirty, so I guess we could also set it somewhere in
> the atomic_helper_check function (similar to how we set the various
> crtc->*_changed flags, but we're not entirely consistent on these either).
> 
> For the overall thing what feels irky to me is making this no_vblank
> default logic (however we end up computing it in the end, whether like
> this or what I suggested) specific to simple pipe helpers feels kinda
> wrong. Simple pipe tends to have a higher ratio of drivers for hw without
> vblank support, but by far not the only ones. Having that special case
> feels confusing to me (and likely will trip up some people, vblank and
> event handling is already a huge source of confusion in drm).

Making it a default for simple KMS was only the start. I intended to
cover all drivers at some point. I just didn't want to go through all
drivers at once.

I guess for the patchset's v3 I'll audit all drivers for the use of
no_blank and drm_crtc_send_vblank_event(); and convert the possible
candidates.

Best regards
Thomas

> 
> One idea behind drm_dev_has_vblank() is also that we could formalize a bit
> all that, at least for the usual case - xen and maybe others being some
> exceptions as usual (hence definitely not something the core code should
> handle).
> 
> Cheers, Daniel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/4] drm/simple-kms: Let DRM core send VBLANK events by default

[Daniel Vetter]

On Fri, Jan 17, 2020 at 08:17:10AM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 17.01.20 um 00:59 schrieb Daniel Vetter:
> > On Thu, Jan 16, 2020 at 05:22:34PM +0000, Emil Velikov wrote:
> >> Hi all,
> >>
> >> On Thu, 16 Jan 2020 at 07:37, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >>
> >>>> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> >>>> index 7cf3cf936547..23d2f51fc1d4 100644
> >>>> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> >>>> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> >>>> @@ -149,6 +149,11 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
> >>>>       /* Self refresh should be canceled when a new update is available */
> >>>>       state->active = drm_atomic_crtc_effectively_active(state);
> >>>>       state->self_refresh_active = false;
> >>>> +
> >>>> +     if (drm_dev_has_vblank(crtc->dev))
> >>>> +             state->no_vblank = true;
> >>>> +     else
> >>>> +             state->no_vblank = false;
> >>>>  }
> >>>>  EXPORT_SYMBOL(__drm_atomic_helper_crtc_duplicate_state);
> >>>
> >>> I think the if/else branches are in the wrong order.
> > 
> > Yeah fumbled that.
> > 
> >>> But generally speaking, is it really that easy? The xen driver already
> >>> has to work around simple-kms's auto-enabling of no_vblank (see patch
> >>> 4). Maybe this settings interferes with other drivers as well. At least
> >>> the calls for sending fake vblanks should be removed from all affected
> >>> drivers.
> > 
> > Hm xen is really special, in that it has a flip complete event, but not a
> > vblank. I think forcing drivers to overwrite stuff in that case makes
> > sense.
> > 
> >> I'm not sure if setting no_vblank based on dev->num_crtcs is the correct thing.
> >> From the original commit and associated description for no_vblank:
> >>
> >> In some cases CRTCs are active but are not able to generating events, at
> >> least not at every frame at it's expected to.
> >> This is typically the case when the CRTC is feeding a writeback connector...
> > 
> > Yeah, but Thomas' series here wants to extend that. And I think if we roll
> > this out the common case will be "no hw vblank", and the writeback special
> 
> Default values should usually be 0 for zalloc and static initializers.
> Should we rename no_vblank to has_vblank then?

Hm, imo feels like hw without vblank is still the uncommon case. I'd leave
this as-is, but also no objections if you feel like repainting :-)

> > case is going to be the exception to the exception. Yup, patch 1 that
> > updates the docs doesn't reflect that, which is why I'm bringing up more
> > suggestions here around code & semantics of all these pieces to make them
> > do the most reasonable thing for most of the drivers.
> > 
> >> Reflects the ability of a CRTC to send VBLANK events....
> >>
> >>
> >> The proposed handling of no_vblank feels a little dirty, although
> >> nothing better comes to mind.
> >> Nevertheless code seems perfectly reasonable, so if it were me I'd merge it.
> > 
> > The idea with setting it very early is that drivers can overwrite it very
> > easily. Feels slightly dirty, so I guess we could also set it somewhere in
> > the atomic_helper_check function (similar to how we set the various
> > crtc->*_changed flags, but we're not entirely consistent on these either).
> > 
> > For the overall thing what feels irky to me is making this no_vblank
> > default logic (however we end up computing it in the end, whether like
> > this or what I suggested) specific to simple pipe helpers feels kinda
> > wrong. Simple pipe tends to have a higher ratio of drivers for hw without
> > vblank support, but by far not the only ones. Having that special case
> > feels confusing to me (and likely will trip up some people, vblank and
> > event handling is already a huge source of confusion in drm).
> 
> Making it a default for simple KMS was only the start. I intended to
> cover all drivers at some point. I just didn't want to go through all
> drivers at once.
> 
> I guess for the patchset's v3 I'll audit all drivers for the use of
> no_blank and drm_crtc_send_vblank_event(); and convert the possible
> candidates.

Yeah it's a pain, thanks for volunteering. Just figured the half-step here
is too much in the uncanney valley. If we're going to polish this, let's
do it right (and we have plenty enough drivers to make sure what we pick
will be a solid choice I think).
-Daniel

> 
> Best regards
> Thomas
> 
> > 
> > One idea behind drm_dev_has_vblank() is also that we could formalize a bit
> > all that, at least for the usual case - xen and maybe others being some
> > exceptions as usual (hence definitely not something the core code should
> > handle).
> > 
> > Cheers, Daniel
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
> 




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/4] drm/simple-kms: Let DRM core send VBLANK events by default

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 5735 bytes --]

Hi

Am 22.01.20 um 09:11 schrieb Daniel Vetter:
> On Fri, Jan 17, 2020 at 08:17:10AM +0100, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 17.01.20 um 00:59 schrieb Daniel Vetter:
>>> On Thu, Jan 16, 2020 at 05:22:34PM +0000, Emil Velikov wrote:
>>>> Hi all,
>>>>
>>>> On Thu, 16 Jan 2020 at 07:37, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
>>>>>> index 7cf3cf936547..23d2f51fc1d4 100644
>>>>>> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
>>>>>> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
>>>>>> @@ -149,6 +149,11 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
>>>>>>       /* Self refresh should be canceled when a new update is available */
>>>>>>       state->active = drm_atomic_crtc_effectively_active(state);
>>>>>>       state->self_refresh_active = false;
>>>>>> +
>>>>>> +     if (drm_dev_has_vblank(crtc->dev))
>>>>>> +             state->no_vblank = true;
>>>>>> +     else
>>>>>> +             state->no_vblank = false;
>>>>>>  }
>>>>>>  EXPORT_SYMBOL(__drm_atomic_helper_crtc_duplicate_state);
>>>>>
>>>>> I think the if/else branches are in the wrong order.
>>>
>>> Yeah fumbled that.
>>>
>>>>> But generally speaking, is it really that easy? The xen driver already
>>>>> has to work around simple-kms's auto-enabling of no_vblank (see patch
>>>>> 4). Maybe this settings interferes with other drivers as well. At least
>>>>> the calls for sending fake vblanks should be removed from all affected
>>>>> drivers.
>>>
>>> Hm xen is really special, in that it has a flip complete event, but not a
>>> vblank. I think forcing drivers to overwrite stuff in that case makes
>>> sense.
>>>
>>>> I'm not sure if setting no_vblank based on dev->num_crtcs is the correct thing.
>>>> From the original commit and associated description for no_vblank:
>>>>
>>>> In some cases CRTCs are active but are not able to generating events, at
>>>> least not at every frame at it's expected to.
>>>> This is typically the case when the CRTC is feeding a writeback connector...
>>>
>>> Yeah, but Thomas' series here wants to extend that. And I think if we roll
>>> this out the common case will be "no hw vblank", and the writeback special
>>
>> Default values should usually be 0 for zalloc and static initializers.
>> Should we rename no_vblank to has_vblank then?
> 
> Hm, imo feels like hw without vblank is still the uncommon case. I'd leave
> this as-is, but also no objections if you feel like repainting :-)

Not a bit. ;)

> 
>>> case is going to be the exception to the exception. Yup, patch 1 that
>>> updates the docs doesn't reflect that, which is why I'm bringing up more
>>> suggestions here around code & semantics of all these pieces to make them
>>> do the most reasonable thing for most of the drivers.
>>>
>>>> Reflects the ability of a CRTC to send VBLANK events....
>>>>
>>>>
>>>> The proposed handling of no_vblank feels a little dirty, although
>>>> nothing better comes to mind.
>>>> Nevertheless code seems perfectly reasonable, so if it were me I'd merge it.
>>>
>>> The idea with setting it very early is that drivers can overwrite it very
>>> easily. Feels slightly dirty, so I guess we could also set it somewhere in
>>> the atomic_helper_check function (similar to how we set the various
>>> crtc->*_changed flags, but we're not entirely consistent on these either).
>>>
>>> For the overall thing what feels irky to me is making this no_vblank
>>> default logic (however we end up computing it in the end, whether like
>>> this or what I suggested) specific to simple pipe helpers feels kinda
>>> wrong. Simple pipe tends to have a higher ratio of drivers for hw without
>>> vblank support, but by far not the only ones. Having that special case
>>> feels confusing to me (and likely will trip up some people, vblank and
>>> event handling is already a huge source of confusion in drm).
>>
>> Making it a default for simple KMS was only the start. I intended to
>> cover all drivers at some point. I just didn't want to go through all
>> drivers at once.
>>
>> I guess for the patchset's v3 I'll audit all drivers for the use of
>> no_blank and drm_crtc_send_vblank_event(); and convert the possible
>> candidates.
> 
> Yeah it's a pain, thanks for volunteering. Just figured the half-step here
> is too much in the uncanney valley. If we're going to polish this, let's
> do it right (and we have plenty enough drivers to make sure what we pick
> will be a solid choice I think).

I went through the drivers and updated them. It's the ones covered here
plus some more virtual HW. My search heuristic was to look for drivers
that call drm_crtc_send_vblank_event() but do not call
drm_vblank_init(). Please see v3 of this patchset. It should be pending
on this ML.

Best regards
Thomas

> -Daniel
> 
>>
>> Best regards
>> Thomas
>>
>>>
>>> One idea behind drm_dev_has_vblank() is also that we could formalize a bit
>>> all that, at least for the usual case - xen and maybe others being some
>>> exceptions as usual (hence definitely not something the core code should
>>> handle).
>>>
>>> Cheers, Daniel
>>>
>>
>> -- 
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Felix Imendörffer
>>
> 
> 
> 
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/4] Use no_vblank property for drivers without VBLANK

[Hans de Goede]

Hi,

On 15-01-2020 13:52, Thomas Zimmermann wrote:
> (Resending because I did not cc dri-devel properly.)
> 
> 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 several drivers.
> 
> Ast already uses the functionality and just needs a cleanup. Cirrus can
> be converted easily by setting the field in the check() callback and
> removing the existing VBLANK code. For most other simple-KMS drivers
> without enable_vblank() and check(), simple-KMS helpers can enable the
> faked VBLANK by default. The only exception is Xen, which comes with
> its own VBLANK logic and should rather to disable no_vblank.
> 
> v2:
> 	* document functionality (Daniel)
> 	* cleanup ast (Daniel)
> 	* let simple-kms handle no_vblank where possible

Entire series looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans




> Thomas Zimmermann (4):
>    drm: Document struct drm_crtc_state.no_vblank for faking VBLANK events
>    drm/ast: Set struct drm_crtc_state.no_vblank in atomic_check()
>    drm/cirrus: Let DRM core send VBLANK events
>    drm/simple-kms: Let DRM core send VBLANK events by default
> 
>   drivers/gpu/drm/ast/ast_mode.c          |  4 ++--
>   drivers/gpu/drm/bochs/bochs_kms.c       |  9 ---------
>   drivers/gpu/drm/cirrus/cirrus.c         | 10 ++--------
>   drivers/gpu/drm/drm_atomic_helper.c     |  4 +++-
>   drivers/gpu/drm/drm_mipi_dbi.c          |  9 ---------
>   drivers/gpu/drm/drm_simple_kms_helper.c | 19 +++++++++++++++----
>   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/xen/xen_drm_front_kms.c | 13 +++++++++++++
>   include/drm/drm_crtc.h                  |  9 +++++++--
>   include/drm/drm_simple_kms_helper.h     |  7 +++++--
>   14 files changed, 47 insertions(+), 84 deletions(-)
> 
> --
> 2.24.1
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel