All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/3] Rate-limit shadow-FB-to-console-update to screen refresh
@ 2019-09-12  6:42 Thomas Zimmermann
  2019-09-12  6:42 ` [PATCH v2 1/3] drm/fb-helper: Synchronize dirty worker with vblank Thomas Zimmermann
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Thomas Zimmermann @ 2019-09-12  6:42 UTC (permalink / raw)
  To: daniel, airlied, noralf, rong.a.chen, feng.tang, ying.huang,
	kraxel, ville.syrjala
  Cc: Thomas Zimmermann, dri-devel

A full-screen memcpy() moves the console's shadow buffer to hardware; with
possibly significant runtime overhead. [1]

The console's dirty worker now waits for the vblank to rate limit the
output frequency. Screen output can pile up while waiting and there's a
chance that multiple screen updates can be handled with a single memcpy().
Note that this has no effect on tearing: while the dirty worker updates
the hardware buffer, new data can still arrive in the shadow buffer. This
can create a tearing effcet, even though console output is synchronized
to vblank.

The patchset adds vblank support to mgag200, because the problem was first
reported with Matrox hardware.

v2:
	* remove locking from fbdev patch
	* use constants fro mgag200 register names and fields
	* double-check that VLINE irq is active on mgag200
	* only signal vblank on CRTC 0 of mgag200
	* coding-style fixes

[1] https://lists.freedesktop.org/archives/dri-devel/2019-July/228663.html

Thomas Zimmermann (3):
  drm/fb-helper: Synchronize dirty worker with vblank
  drm/mgag200: Rename constant MGAREG_Status to MGAREG_STATUS
  drm/mgag200: Add vblank support

 drivers/gpu/drm/drm_fb_helper.c        | 10 +++++
 drivers/gpu/drm/mgag200/mgag200_drv.c  |  1 +
 drivers/gpu/drm/mgag200/mgag200_drv.h  |  1 +
 drivers/gpu/drm/mgag200/mgag200_main.c | 40 +++++++++++++++++++
 drivers/gpu/drm/mgag200/mgag200_mode.c | 53 ++++++++++++++++++++++----
 drivers/gpu/drm/mgag200/mgag200_reg.h  |  7 +++-
 6 files changed, 104 insertions(+), 8 deletions(-)

--
2.23.0

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/3] drm/fb-helper: Synchronize dirty worker with vblank
  2019-09-12  6:42 [PATCH v2 0/3] Rate-limit shadow-FB-to-console-update to screen refresh Thomas Zimmermann
@ 2019-09-12  6:42 ` Thomas Zimmermann
  2019-09-12  8:52   ` Gerd Hoffmann
  2019-09-17 13:06   ` Daniel Vetter
  2019-09-12  6:42 ` [PATCH v2 2/3] drm/mgag200: Rename constant MGAREG_Status to MGAREG_STATUS Thomas Zimmermann
  2019-09-12  6:42 ` [PATCH v2 3/3] drm/mgag200: Add vblank support Thomas Zimmermann
  2 siblings, 2 replies; 10+ messages in thread
From: Thomas Zimmermann @ 2019-09-12  6:42 UTC (permalink / raw)
  To: daniel, airlied, noralf, rong.a.chen, feng.tang, ying.huang,
	kraxel, ville.syrjala
  Cc: Thomas Zimmermann, dri-devel

Before updating the display from the console's shadow buffer, the dirty
worker now waits for vblank. This allows several screen updates to pile
up and acts as a rate limiter.

v2:
	* don't hold helper->lock while waiting for vblank

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

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index a7ba5b4902d6..d0cb1fa4f909 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -402,8 +402,18 @@ static void drm_fb_helper_dirty_work(struct work_struct *work)
 						    dirty_work);
 	struct drm_clip_rect *clip = &helper->dirty_clip;
 	struct drm_clip_rect clip_copy;
+	struct drm_crtc *crtc;
 	unsigned long flags;
 	void *vaddr;
+	int ret;
+
+	/* rate-limit update frequency */
+	crtc = helper->client.modesets[0].crtc;
+	ret = drm_crtc_vblank_get(crtc);
+	if (!ret) {
+		drm_crtc_wait_one_vblank(crtc);
+		drm_crtc_vblank_put(crtc);
+	}
 
 	spin_lock_irqsave(&helper->dirty_lock, flags);
 	clip_copy = *clip;
-- 
2.23.0

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/3] drm/mgag200: Rename constant MGAREG_Status to MGAREG_STATUS
  2019-09-12  6:42 [PATCH v2 0/3] Rate-limit shadow-FB-to-console-update to screen refresh Thomas Zimmermann
  2019-09-12  6:42 ` [PATCH v2 1/3] drm/fb-helper: Synchronize dirty worker with vblank Thomas Zimmermann
@ 2019-09-12  6:42 ` Thomas Zimmermann
  2019-09-12  8:52   ` Gerd Hoffmann
  2019-09-12  6:42 ` [PATCH v2 3/3] drm/mgag200: Add vblank support Thomas Zimmermann
  2 siblings, 1 reply; 10+ messages in thread
From: Thomas Zimmermann @ 2019-09-12  6:42 UTC (permalink / raw)
  To: daniel, airlied, noralf, rong.a.chen, feng.tang, ying.huang,
	kraxel, ville.syrjala
  Cc: Thomas Zimmermann, dri-devel

Register constants are upper case. Fix MGAREG_Status accordingly.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 6 +++---
 drivers/gpu/drm/mgag200/mgag200_reg.h  | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 5e778b5f1a10..302ba40eb033 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -79,12 +79,12 @@ static inline void mga_wait_vsync(struct mga_device *mdev)
 	unsigned int status = 0;
 
 	do {
-		status = RREG32(MGAREG_Status);
+		status = RREG32(MGAREG_STATUS);
 	} while ((status & 0x08) && time_before(jiffies, timeout));
 	timeout = jiffies + HZ/10;
 	status = 0;
 	do {
-		status = RREG32(MGAREG_Status);
+		status = RREG32(MGAREG_STATUS);
 	} while (!(status & 0x08) && time_before(jiffies, timeout));
 }
 
@@ -93,7 +93,7 @@ static inline void mga_wait_busy(struct mga_device *mdev)
 	unsigned long timeout = jiffies + HZ;
 	unsigned int status = 0;
 	do {
-		status = RREG8(MGAREG_Status + 2);
+		status = RREG8(MGAREG_STATUS + 2);
 	} while ((status & 0x01) && time_before(jiffies, timeout));
 }
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_reg.h b/drivers/gpu/drm/mgag200/mgag200_reg.h
index c096a9d6bcbc..6c460d9a2143 100644
--- a/drivers/gpu/drm/mgag200/mgag200_reg.h
+++ b/drivers/gpu/drm/mgag200/mgag200_reg.h
@@ -101,7 +101,7 @@
 #define MGAREG_EXEC		0x0100
 
 #define	MGAREG_FIFOSTATUS	0x1e10
-#define	MGAREG_Status		0x1e14
+#define	MGAREG_STATUS		0x1e14
 #define MGAREG_CACHEFLUSH       0x1fff
 #define	MGAREG_ICLEAR		0x1e18
 #define	MGAREG_IEN		0x1e1c
-- 
2.23.0

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 3/3] drm/mgag200: Add vblank support
  2019-09-12  6:42 [PATCH v2 0/3] Rate-limit shadow-FB-to-console-update to screen refresh Thomas Zimmermann
  2019-09-12  6:42 ` [PATCH v2 1/3] drm/fb-helper: Synchronize dirty worker with vblank Thomas Zimmermann
  2019-09-12  6:42 ` [PATCH v2 2/3] drm/mgag200: Rename constant MGAREG_Status to MGAREG_STATUS Thomas Zimmermann
@ 2019-09-12  6:42 ` Thomas Zimmermann
  2019-09-12  8:53   ` Gerd Hoffmann
  2 siblings, 1 reply; 10+ messages in thread
From: Thomas Zimmermann @ 2019-09-12  6:42 UTC (permalink / raw)
  To: daniel, airlied, noralf, rong.a.chen, feng.tang, ying.huang,
	kraxel, ville.syrjala
  Cc: Thomas Zimmermann, dri-devel

There's no VBLANK interrupt on Matrox chipsets. The workaround that is
being used here and in other free Matrox drivers is to program <linecomp>
to the value of <vdisplay> and enable the VLINE interrupt. This triggers
an interrupt at the time when VBLANK begins.

VLINE uses separate registers for enabling and clearing pending interrupts.
No extra syncronization between irq handler and the rest of the driver is
required.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

v2:
	* only signal vblank on CRTC 0
	* use constants for registers and fields
	* set VLINECLR before enabling interrupt
	* test against STATUS and IEN in irq handler
	* coding-style fixes
---
 drivers/gpu/drm/mgag200/mgag200_drv.c  |  1 +
 drivers/gpu/drm/mgag200/mgag200_drv.h  |  1 +
 drivers/gpu/drm/mgag200/mgag200_main.c | 40 ++++++++++++++++++++++
 drivers/gpu/drm/mgag200/mgag200_mode.c | 47 +++++++++++++++++++++++---
 drivers/gpu/drm/mgag200/mgag200_reg.h  |  5 +++
 5 files changed, 90 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 4f9df3b93598..cff265973154 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -67,6 +67,7 @@ static struct drm_driver driver = {
 	.driver_features = DRIVER_GEM | DRIVER_MODESET,
 	.load = mgag200_driver_load,
 	.unload = mgag200_driver_unload,
+	.irq_handler = mgag200_irq_handler,
 	.fops = &mgag200_driver_fops,
 	.name = DRIVER_NAME,
 	.desc = DRIVER_DESC,
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 1c93f8dc08c7..88cf256d135f 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -195,6 +195,7 @@ void mgag200_modeset_fini(struct mga_device *mdev);
 				/* mgag200_main.c */
 int mgag200_driver_load(struct drm_device *dev, unsigned long flags);
 void mgag200_driver_unload(struct drm_device *dev);
+irqreturn_t mgag200_irq_handler(int irq, void *arg);
 
 				/* mgag200_i2c.c */
 struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev);
diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
index a9773334dedf..44273a66f5a5 100644
--- a/drivers/gpu/drm/mgag200/mgag200_main.c
+++ b/drivers/gpu/drm/mgag200/mgag200_main.c
@@ -10,7 +10,9 @@
 
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_irq.h>
 #include <drm/drm_pci.h>
+#include <drm/drm_vblank.h>
 
 #include "mgag200_drv.h"
 
@@ -186,10 +188,18 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
 	}
 	mdev->cursor.pixels_current = NULL;
 
+	r = drm_vblank_init(dev, 1);
+	if (r)
+		goto err_modeset;
+
 	r = drm_fbdev_generic_setup(mdev->dev, 0);
 	if (r)
 		goto err_modeset;
 
+	r = drm_irq_install(dev, dev->pdev->irq);
+	if (r)
+		goto err_modeset;
+
 	return 0;
 
 err_modeset:
@@ -207,8 +217,38 @@ void mgag200_driver_unload(struct drm_device *dev)
 
 	if (mdev == NULL)
 		return;
+	drm_irq_uninstall(dev);
 	mgag200_modeset_fini(mdev);
 	drm_mode_config_cleanup(dev);
 	mgag200_mm_fini(mdev);
 	dev->dev_private = NULL;
 }
+
+irqreturn_t mgag200_irq_handler(int irq, void *arg)
+{
+	struct drm_device *dev = arg;
+	struct mga_device *mdev = dev->dev_private;
+	struct drm_crtc *crtc;
+	u32 status, ien, iclear;
+
+	status = RREG32(MGAREG_STATUS);
+
+	if (status & MGA_VLINEPEN) {
+		ien = RREG32(MGAREG_IEN);
+		if (!(ien & MGA_VLINEIEN))
+			goto out;
+
+		crtc = drm_crtc_from_index(dev, 0);
+		if (WARN_ON_ONCE(!crtc))
+			goto out;
+		drm_crtc_handle_vblank(crtc);
+
+		iclear = RREG32(MGAREG_ICLEAR);
+		iclear |= MGA_VLINECLR;
+		WREG32(MGAREG_ICLEAR, iclear);
+		return IRQ_HANDLED;
+	}
+
+out:
+	return IRQ_NONE;
+};
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 302ba40eb033..e13c3244fea9 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -905,6 +905,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
 	const struct drm_framebuffer *fb = crtc->primary->fb;
 	int hdisplay, hsyncstart, hsyncend, htotal;
 	int vdisplay, vsyncstart, vsyncend, vtotal;
+	int linecomp;
 	int pitch;
 	int option = 0, option2 = 0;
 	int i;
@@ -1042,6 +1043,14 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
 	vsyncend = mode->vsync_end - 1;
 	vtotal = mode->vtotal - 2;
 
+	/*
+	 * There's no VBLANK interrupt on Matrox chipsets, so we have to
+	 * use the VLINE interrupt instead. It triggers when the current
+	 * linecomp has been reached. Therefore keep <linecomp> in
+	 * sync with <vdisplay>.
+	 */
+	linecomp = vdisplay;
+
 	WREG_GFX(0, 0);
 	WREG_GFX(1, 0);
 	WREG_GFX(2, 0);
@@ -1063,12 +1072,12 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
 		 ((vdisplay & 0x100) >> 7) |
 		 ((vsyncstart & 0x100) >> 6) |
 		 ((vdisplay & 0x100) >> 5) |
-		 ((vdisplay & 0x100) >> 4) | /* linecomp */
+		 ((linecomp & 0x100) >> 4) |
 		 ((vtotal & 0x200) >> 4)|
 		 ((vdisplay & 0x200) >> 3) |
 		 ((vsyncstart & 0x200) >> 2));
 	WREG_CRT(9, ((vdisplay & 0x200) >> 4) |
-		 ((vdisplay & 0x200) >> 3));
+		 ((linecomp & 0x200) >> 3));
 	WREG_CRT(10, 0);
 	WREG_CRT(11, 0);
 	WREG_CRT(12, 0);
@@ -1083,7 +1092,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
 	WREG_CRT(21, vdisplay & 0xFF);
 	WREG_CRT(22, (vtotal + 1) & 0xFF);
 	WREG_CRT(23, 0xc3);
-	WREG_CRT(24, vdisplay & 0xFF);
+	WREG_CRT(24, linecomp & 0xff);
 
 	ext_vga[0] = 0;
 	ext_vga[5] = 0;
@@ -1099,7 +1108,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
 		((vdisplay & 0x400) >> 8) |
 		((vdisplay & 0xc00) >> 7) |
 		((vsyncstart & 0xc00) >> 5) |
-		((vdisplay & 0x400) >> 3);
+		((linecomp & 0x400) >> 3);
 	if (fb->format->cpp[0] * 8 == 24)
 		ext_vga[3] = (((1 << bppshift) * 3) - 1) | 0x80;
 	else
@@ -1411,6 +1420,34 @@ static void mga_crtc_disable(struct drm_crtc *crtc)
 	crtc->primary->fb = NULL;
 }
 
+static int mga_crtc_enable_vblank(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct mga_device *mdev = dev->dev_private;
+	u32 iclear, ien;
+
+	iclear = RREG32(MGAREG_ICLEAR);
+	iclear |= MGA_VLINECLR;
+	WREG32(MGAREG_ICLEAR, iclear);
+
+	ien = RREG32(MGAREG_IEN);
+	ien |= MGA_VLINEIEN;
+	WREG32(MGAREG_IEN, ien);
+
+	return 0;
+}
+
+static void mga_crtc_disable_vblank(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct mga_device *mdev = dev->dev_private;
+	u32 ien;
+
+	ien = RREG32(MGAREG_IEN);
+	ien &= ~(MGA_VLINEIEN);
+	WREG32(MGAREG_IEN, ien);
+}
+
 /* These provide the minimum set of functions required to handle a CRTC */
 static const struct drm_crtc_funcs mga_crtc_funcs = {
 	.cursor_set = mga_crtc_cursor_set,
@@ -1418,6 +1455,8 @@ static const struct drm_crtc_funcs mga_crtc_funcs = {
 	.gamma_set = mga_crtc_gamma_set,
 	.set_config = drm_crtc_helper_set_config,
 	.destroy = mga_crtc_destroy,
+	.enable_vblank = mga_crtc_enable_vblank,
+	.disable_vblank = mga_crtc_disable_vblank,
 };
 
 static const struct drm_crtc_helper_funcs mga_helper_funcs = {
diff --git a/drivers/gpu/drm/mgag200/mgag200_reg.h b/drivers/gpu/drm/mgag200/mgag200_reg.h
index 6c460d9a2143..44db1d8279fa 100644
--- a/drivers/gpu/drm/mgag200/mgag200_reg.h
+++ b/drivers/gpu/drm/mgag200/mgag200_reg.h
@@ -122,6 +122,11 @@
 
 #define MGAREG_MEMCTL           0x2e08
 
+/* Interrupt fields */
+#define MGA_VLINEPEN		(0x01 << 5)
+#define MGA_VLINECLR		(0x01 << 5)
+#define MGA_VLINEIEN		(0x01 << 5)
+
 /* OPMODE register additives */
 
 #define MGAOPM_DMA_GENERAL	(0x00 << 2)
-- 
2.23.0

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/3] drm/fb-helper: Synchronize dirty worker with vblank
  2019-09-12  6:42 ` [PATCH v2 1/3] drm/fb-helper: Synchronize dirty worker with vblank Thomas Zimmermann
@ 2019-09-12  8:52   ` Gerd Hoffmann
  2019-09-17 13:06   ` Daniel Vetter
  1 sibling, 0 replies; 10+ messages in thread
From: Gerd Hoffmann @ 2019-09-12  8:52 UTC (permalink / raw)
  To: Thomas Zimmermann; +Cc: feng.tang, rong.a.chen, dri-devel, ying.huang, airlied

On Thu, Sep 12, 2019 at 08:42:28AM +0200, Thomas Zimmermann wrote:
> Before updating the display from the console's shadow buffer, the dirty
> worker now waits for vblank. This allows several screen updates to pile
> up and acts as a rate limiter.
> 
> v2:
> 	* don't hold helper->lock while waiting for vblank
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>#

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

> ---
>  drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index a7ba5b4902d6..d0cb1fa4f909 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -402,8 +402,18 @@ static void drm_fb_helper_dirty_work(struct work_struct *work)
>  						    dirty_work);
>  	struct drm_clip_rect *clip = &helper->dirty_clip;
>  	struct drm_clip_rect clip_copy;
> +	struct drm_crtc *crtc;
>  	unsigned long flags;
>  	void *vaddr;
> +	int ret;
> +
> +	/* rate-limit update frequency */
> +	crtc = helper->client.modesets[0].crtc;
> +	ret = drm_crtc_vblank_get(crtc);
> +	if (!ret) {
> +		drm_crtc_wait_one_vblank(crtc);
> +		drm_crtc_vblank_put(crtc);
> +	}
>  
>  	spin_lock_irqsave(&helper->dirty_lock, flags);
>  	clip_copy = *clip;
> -- 
> 2.23.0
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/3] drm/mgag200: Rename constant MGAREG_Status to MGAREG_STATUS
  2019-09-12  6:42 ` [PATCH v2 2/3] drm/mgag200: Rename constant MGAREG_Status to MGAREG_STATUS Thomas Zimmermann
@ 2019-09-12  8:52   ` Gerd Hoffmann
  0 siblings, 0 replies; 10+ messages in thread
From: Gerd Hoffmann @ 2019-09-12  8:52 UTC (permalink / raw)
  To: Thomas Zimmermann; +Cc: feng.tang, rong.a.chen, dri-devel, ying.huang, airlied

On Thu, Sep 12, 2019 at 08:42:29AM +0200, Thomas Zimmermann wrote:
> Register constants are upper case. Fix MGAREG_Status accordingly.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

> ---
>  drivers/gpu/drm/mgag200/mgag200_mode.c | 6 +++---
>  drivers/gpu/drm/mgag200/mgag200_reg.h  | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index 5e778b5f1a10..302ba40eb033 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -79,12 +79,12 @@ static inline void mga_wait_vsync(struct mga_device *mdev)
>  	unsigned int status = 0;
>  
>  	do {
> -		status = RREG32(MGAREG_Status);
> +		status = RREG32(MGAREG_STATUS);
>  	} while ((status & 0x08) && time_before(jiffies, timeout));
>  	timeout = jiffies + HZ/10;
>  	status = 0;
>  	do {
> -		status = RREG32(MGAREG_Status);
> +		status = RREG32(MGAREG_STATUS);
>  	} while (!(status & 0x08) && time_before(jiffies, timeout));
>  }
>  
> @@ -93,7 +93,7 @@ static inline void mga_wait_busy(struct mga_device *mdev)
>  	unsigned long timeout = jiffies + HZ;
>  	unsigned int status = 0;
>  	do {
> -		status = RREG8(MGAREG_Status + 2);
> +		status = RREG8(MGAREG_STATUS + 2);
>  	} while ((status & 0x01) && time_before(jiffies, timeout));
>  }
>  
> diff --git a/drivers/gpu/drm/mgag200/mgag200_reg.h b/drivers/gpu/drm/mgag200/mgag200_reg.h
> index c096a9d6bcbc..6c460d9a2143 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_reg.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_reg.h
> @@ -101,7 +101,7 @@
>  #define MGAREG_EXEC		0x0100
>  
>  #define	MGAREG_FIFOSTATUS	0x1e10
> -#define	MGAREG_Status		0x1e14
> +#define	MGAREG_STATUS		0x1e14
>  #define MGAREG_CACHEFLUSH       0x1fff
>  #define	MGAREG_ICLEAR		0x1e18
>  #define	MGAREG_IEN		0x1e1c
> -- 
> 2.23.0
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/3] drm/mgag200: Add vblank support
  2019-09-12  6:42 ` [PATCH v2 3/3] drm/mgag200: Add vblank support Thomas Zimmermann
@ 2019-09-12  8:53   ` Gerd Hoffmann
  0 siblings, 0 replies; 10+ messages in thread
From: Gerd Hoffmann @ 2019-09-12  8:53 UTC (permalink / raw)
  To: Thomas Zimmermann; +Cc: feng.tang, rong.a.chen, dri-devel, ying.huang, airlied

On Thu, Sep 12, 2019 at 08:42:30AM +0200, Thomas Zimmermann wrote:
> There's no VBLANK interrupt on Matrox chipsets. The workaround that is
> being used here and in other free Matrox drivers is to program <linecomp>
> to the value of <vdisplay> and enable the VLINE interrupt. This triggers
> an interrupt at the time when VBLANK begins.
> 
> VLINE uses separate registers for enabling and clearing pending interrupts.
> No extra syncronization between irq handler and the rest of the driver is
> required.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Looks sane, can't justify on mga hardware details though.

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

> 
> v2:
> 	* only signal vblank on CRTC 0
> 	* use constants for registers and fields
> 	* set VLINECLR before enabling interrupt
> 	* test against STATUS and IEN in irq handler
> 	* coding-style fixes
> ---
>  drivers/gpu/drm/mgag200/mgag200_drv.c  |  1 +
>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  1 +
>  drivers/gpu/drm/mgag200/mgag200_main.c | 40 ++++++++++++++++++++++
>  drivers/gpu/drm/mgag200/mgag200_mode.c | 47 +++++++++++++++++++++++---
>  drivers/gpu/drm/mgag200/mgag200_reg.h  |  5 +++
>  5 files changed, 90 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
> index 4f9df3b93598..cff265973154 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> @@ -67,6 +67,7 @@ static struct drm_driver driver = {
>  	.driver_features = DRIVER_GEM | DRIVER_MODESET,
>  	.load = mgag200_driver_load,
>  	.unload = mgag200_driver_unload,
> +	.irq_handler = mgag200_irq_handler,
>  	.fops = &mgag200_driver_fops,
>  	.name = DRIVER_NAME,
>  	.desc = DRIVER_DESC,
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> index 1c93f8dc08c7..88cf256d135f 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> @@ -195,6 +195,7 @@ void mgag200_modeset_fini(struct mga_device *mdev);
>  				/* mgag200_main.c */
>  int mgag200_driver_load(struct drm_device *dev, unsigned long flags);
>  void mgag200_driver_unload(struct drm_device *dev);
> +irqreturn_t mgag200_irq_handler(int irq, void *arg);
>  
>  				/* mgag200_i2c.c */
>  struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
> index a9773334dedf..44273a66f5a5 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
> @@ -10,7 +10,9 @@
>  
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_irq.h>
>  #include <drm/drm_pci.h>
> +#include <drm/drm_vblank.h>
>  
>  #include "mgag200_drv.h"
>  
> @@ -186,10 +188,18 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>  	}
>  	mdev->cursor.pixels_current = NULL;
>  
> +	r = drm_vblank_init(dev, 1);
> +	if (r)
> +		goto err_modeset;
> +
>  	r = drm_fbdev_generic_setup(mdev->dev, 0);
>  	if (r)
>  		goto err_modeset;
>  
> +	r = drm_irq_install(dev, dev->pdev->irq);
> +	if (r)
> +		goto err_modeset;
> +
>  	return 0;
>  
>  err_modeset:
> @@ -207,8 +217,38 @@ void mgag200_driver_unload(struct drm_device *dev)
>  
>  	if (mdev == NULL)
>  		return;
> +	drm_irq_uninstall(dev);
>  	mgag200_modeset_fini(mdev);
>  	drm_mode_config_cleanup(dev);
>  	mgag200_mm_fini(mdev);
>  	dev->dev_private = NULL;
>  }
> +
> +irqreturn_t mgag200_irq_handler(int irq, void *arg)
> +{
> +	struct drm_device *dev = arg;
> +	struct mga_device *mdev = dev->dev_private;
> +	struct drm_crtc *crtc;
> +	u32 status, ien, iclear;
> +
> +	status = RREG32(MGAREG_STATUS);
> +
> +	if (status & MGA_VLINEPEN) {
> +		ien = RREG32(MGAREG_IEN);
> +		if (!(ien & MGA_VLINEIEN))
> +			goto out;
> +
> +		crtc = drm_crtc_from_index(dev, 0);
> +		if (WARN_ON_ONCE(!crtc))
> +			goto out;
> +		drm_crtc_handle_vblank(crtc);
> +
> +		iclear = RREG32(MGAREG_ICLEAR);
> +		iclear |= MGA_VLINECLR;
> +		WREG32(MGAREG_ICLEAR, iclear);
> +		return IRQ_HANDLED;
> +	}
> +
> +out:
> +	return IRQ_NONE;
> +};
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index 302ba40eb033..e13c3244fea9 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -905,6 +905,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>  	const struct drm_framebuffer *fb = crtc->primary->fb;
>  	int hdisplay, hsyncstart, hsyncend, htotal;
>  	int vdisplay, vsyncstart, vsyncend, vtotal;
> +	int linecomp;
>  	int pitch;
>  	int option = 0, option2 = 0;
>  	int i;
> @@ -1042,6 +1043,14 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>  	vsyncend = mode->vsync_end - 1;
>  	vtotal = mode->vtotal - 2;
>  
> +	/*
> +	 * There's no VBLANK interrupt on Matrox chipsets, so we have to
> +	 * use the VLINE interrupt instead. It triggers when the current
> +	 * linecomp has been reached. Therefore keep <linecomp> in
> +	 * sync with <vdisplay>.
> +	 */
> +	linecomp = vdisplay;
> +
>  	WREG_GFX(0, 0);
>  	WREG_GFX(1, 0);
>  	WREG_GFX(2, 0);
> @@ -1063,12 +1072,12 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>  		 ((vdisplay & 0x100) >> 7) |
>  		 ((vsyncstart & 0x100) >> 6) |
>  		 ((vdisplay & 0x100) >> 5) |
> -		 ((vdisplay & 0x100) >> 4) | /* linecomp */
> +		 ((linecomp & 0x100) >> 4) |
>  		 ((vtotal & 0x200) >> 4)|
>  		 ((vdisplay & 0x200) >> 3) |
>  		 ((vsyncstart & 0x200) >> 2));
>  	WREG_CRT(9, ((vdisplay & 0x200) >> 4) |
> -		 ((vdisplay & 0x200) >> 3));
> +		 ((linecomp & 0x200) >> 3));
>  	WREG_CRT(10, 0);
>  	WREG_CRT(11, 0);
>  	WREG_CRT(12, 0);
> @@ -1083,7 +1092,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>  	WREG_CRT(21, vdisplay & 0xFF);
>  	WREG_CRT(22, (vtotal + 1) & 0xFF);
>  	WREG_CRT(23, 0xc3);
> -	WREG_CRT(24, vdisplay & 0xFF);
> +	WREG_CRT(24, linecomp & 0xff);
>  
>  	ext_vga[0] = 0;
>  	ext_vga[5] = 0;
> @@ -1099,7 +1108,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>  		((vdisplay & 0x400) >> 8) |
>  		((vdisplay & 0xc00) >> 7) |
>  		((vsyncstart & 0xc00) >> 5) |
> -		((vdisplay & 0x400) >> 3);
> +		((linecomp & 0x400) >> 3);
>  	if (fb->format->cpp[0] * 8 == 24)
>  		ext_vga[3] = (((1 << bppshift) * 3) - 1) | 0x80;
>  	else
> @@ -1411,6 +1420,34 @@ static void mga_crtc_disable(struct drm_crtc *crtc)
>  	crtc->primary->fb = NULL;
>  }
>  
> +static int mga_crtc_enable_vblank(struct drm_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct mga_device *mdev = dev->dev_private;
> +	u32 iclear, ien;
> +
> +	iclear = RREG32(MGAREG_ICLEAR);
> +	iclear |= MGA_VLINECLR;
> +	WREG32(MGAREG_ICLEAR, iclear);
> +
> +	ien = RREG32(MGAREG_IEN);
> +	ien |= MGA_VLINEIEN;
> +	WREG32(MGAREG_IEN, ien);
> +
> +	return 0;
> +}
> +
> +static void mga_crtc_disable_vblank(struct drm_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct mga_device *mdev = dev->dev_private;
> +	u32 ien;
> +
> +	ien = RREG32(MGAREG_IEN);
> +	ien &= ~(MGA_VLINEIEN);
> +	WREG32(MGAREG_IEN, ien);
> +}
> +
>  /* These provide the minimum set of functions required to handle a CRTC */
>  static const struct drm_crtc_funcs mga_crtc_funcs = {
>  	.cursor_set = mga_crtc_cursor_set,
> @@ -1418,6 +1455,8 @@ static const struct drm_crtc_funcs mga_crtc_funcs = {
>  	.gamma_set = mga_crtc_gamma_set,
>  	.set_config = drm_crtc_helper_set_config,
>  	.destroy = mga_crtc_destroy,
> +	.enable_vblank = mga_crtc_enable_vblank,
> +	.disable_vblank = mga_crtc_disable_vblank,
>  };
>  
>  static const struct drm_crtc_helper_funcs mga_helper_funcs = {
> diff --git a/drivers/gpu/drm/mgag200/mgag200_reg.h b/drivers/gpu/drm/mgag200/mgag200_reg.h
> index 6c460d9a2143..44db1d8279fa 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_reg.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_reg.h
> @@ -122,6 +122,11 @@
>  
>  #define MGAREG_MEMCTL           0x2e08
>  
> +/* Interrupt fields */
> +#define MGA_VLINEPEN		(0x01 << 5)
> +#define MGA_VLINECLR		(0x01 << 5)
> +#define MGA_VLINEIEN		(0x01 << 5)
> +
>  /* OPMODE register additives */
>  
>  #define MGAOPM_DMA_GENERAL	(0x00 << 2)
> -- 
> 2.23.0
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/3] drm/fb-helper: Synchronize dirty worker with vblank
  2019-09-12  6:42 ` [PATCH v2 1/3] drm/fb-helper: Synchronize dirty worker with vblank Thomas Zimmermann
  2019-09-12  8:52   ` Gerd Hoffmann
@ 2019-09-17 13:06   ` Daniel Vetter
  2019-09-27  8:41     ` Thomas Zimmermann
  1 sibling, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2019-09-17 13:06 UTC (permalink / raw)
  To: Thomas Zimmermann
  Cc: feng.tang, rong.a.chen, dri-devel, kraxel, ying.huang, airlied

On Thu, Sep 12, 2019 at 08:42:28AM +0200, Thomas Zimmermann wrote:
> Before updating the display from the console's shadow buffer, the dirty
> worker now waits for vblank. This allows several screen updates to pile
> up and acts as a rate limiter.
> 
> v2:
> 	* don't hold helper->lock while waiting for vblank
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index a7ba5b4902d6..d0cb1fa4f909 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -402,8 +402,18 @@ static void drm_fb_helper_dirty_work(struct work_struct *work)
>  						    dirty_work);
>  	struct drm_clip_rect *clip = &helper->dirty_clip;
>  	struct drm_clip_rect clip_copy;
> +	struct drm_crtc *crtc;
>  	unsigned long flags;
>  	void *vaddr;
> +	int ret;
> +
> +	/* rate-limit update frequency */
> +	crtc = helper->client.modesets[0].crtc;
> +	ret = drm_crtc_vblank_get(crtc);
> +	if (!ret) {
> +		drm_crtc_wait_one_vblank(crtc);

Without the locking (specifically, preventing other masters) this can go
boom since it again calls drm_vblank_get. If someone turned of the display
meanwhile that will fail, and result in an unsightly WARN backtrace.

I think we need a __drm_crtc_wait_one_vblank(crtc); which requires that
callers hold a full vblank reference already. The other issue is that we
might race with the disabling and hit the timeout, which again gives us an
unsightly WARNING backtrace. Both can happen without locks (that's why the
ioctl path needs them), but we need to avoid.
-Daniel
> +		drm_crtc_vblank_put(crtc);
> +	}
>  
>  	spin_lock_irqsave(&helper->dirty_lock, flags);
>  	clip_copy = *clip;
> -- 
> 2.23.0
> 

-- 
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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/3] drm/fb-helper: Synchronize dirty worker with vblank
  2019-09-17 13:06   ` Daniel Vetter
@ 2019-09-27  8:41     ` Thomas Zimmermann
  2019-10-08 10:01       ` Daniel Vetter
  0 siblings, 1 reply; 10+ messages in thread
From: Thomas Zimmermann @ 2019-09-27  8:41 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: feng.tang, rong.a.chen, dri-devel, kraxel, ying.huang, airlied

Hi

Am 17.09.19 um 15:06 schrieb Daniel Vetter:
> On Thu, Sep 12, 2019 at 08:42:28AM +0200, Thomas Zimmermann wrote:
>> Before updating the display from the console's shadow buffer, the dirty
>> worker now waits for vblank. This allows several screen updates to pile
>> up and acts as a rate limiter.
>>
>> v2:
>> 	* don't hold helper->lock while waiting for vblank
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index a7ba5b4902d6..d0cb1fa4f909 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -402,8 +402,18 @@ static void drm_fb_helper_dirty_work(struct work_struct *work)
>>   						    dirty_work);
>>   	struct drm_clip_rect *clip = &helper->dirty_clip;
>>   	struct drm_clip_rect clip_copy;
>> +	struct drm_crtc *crtc;
>>   	unsigned long flags;
>>   	void *vaddr;
>> +	int ret;
>> +
>> +	/* rate-limit update frequency */
>> +	crtc = helper->client.modesets[0].crtc;
>> +	ret = drm_crtc_vblank_get(crtc);
>> +	if (!ret) {
>> +		drm_crtc_wait_one_vblank(crtc);
> 
> Without the locking (specifically, preventing other masters) this can go
> boom since it again calls drm_vblank_get. If someone turned of the display
> meanwhile that will fail, and result in an unsightly WARN backtrace.
> 
> I think we need a __drm_crtc_wait_one_vblank(crtc); which requires that
> callers hold a full vblank reference already. The other issue is that we
> might race with the disabling and hit the timeout, which again gives us an
> unsightly WARNING backtrace. Both can happen without locks (that's why the
> ioctl path needs them), but we need to avoid.

Here's a status update: I've been working on this patch for a while, but 
the worker still cannot wait reliable for vblanks. When the worker runs, 
the display could be off and hence no vblank events occur. That's 
especially a problem during boot. The worker warns about missed vblanks 
because the console's video mode is still being programmed.

Best regards
Thomas

> -Daniel
>> +		drm_crtc_vblank_put(crtc);
>> +	}
>>   
>>   	spin_lock_irqsave(&helper->dirty_lock, flags);
>>   	clip_copy = *clip;
>> -- 
>> 2.23.0
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/3] drm/fb-helper: Synchronize dirty worker with vblank
  2019-09-27  8:41     ` Thomas Zimmermann
@ 2019-10-08 10:01       ` Daniel Vetter
  0 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2019-10-08 10:01 UTC (permalink / raw)
  To: Thomas Zimmermann
  Cc: feng.tang, rong.a.chen, dri-devel, kraxel, ying.huang, airlied

On Fri, Sep 27, 2019 at 10:41:43AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 17.09.19 um 15:06 schrieb Daniel Vetter:
> > On Thu, Sep 12, 2019 at 08:42:28AM +0200, Thomas Zimmermann wrote:
> > > Before updating the display from the console's shadow buffer, the dirty
> > > worker now waits for vblank. This allows several screen updates to pile
> > > up and acts as a rate limiter.
> > > 
> > > v2:
> > > 	* don't hold helper->lock while waiting for vblank
> > > 
> > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > > ---
> > >   drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++++
> > >   1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > > index a7ba5b4902d6..d0cb1fa4f909 100644
> > > --- a/drivers/gpu/drm/drm_fb_helper.c
> > > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > > @@ -402,8 +402,18 @@ static void drm_fb_helper_dirty_work(struct work_struct *work)
> > >   						    dirty_work);
> > >   	struct drm_clip_rect *clip = &helper->dirty_clip;
> > >   	struct drm_clip_rect clip_copy;
> > > +	struct drm_crtc *crtc;
> > >   	unsigned long flags;
> > >   	void *vaddr;
> > > +	int ret;
> > > +
> > > +	/* rate-limit update frequency */
> > > +	crtc = helper->client.modesets[0].crtc;
> > > +	ret = drm_crtc_vblank_get(crtc);
> > > +	if (!ret) {
> > > +		drm_crtc_wait_one_vblank(crtc);
> > 
> > Without the locking (specifically, preventing other masters) this can go
> > boom since it again calls drm_vblank_get. If someone turned of the display
> > meanwhile that will fail, and result in an unsightly WARN backtrace.
> > 
> > I think we need a __drm_crtc_wait_one_vblank(crtc); which requires that
> > callers hold a full vblank reference already. The other issue is that we
> > might race with the disabling and hit the timeout, which again gives us an
> > unsightly WARNING backtrace. Both can happen without locks (that's why the
> > ioctl path needs them), but we need to avoid.
> 
> Here's a status update: I've been working on this patch for a while, but the
> worker still cannot wait reliable for vblanks. When the worker runs, the
> display could be off and hence no vblank events occur. That's especially a
> problem during boot. The worker warns about missed vblanks because the
> console's video mode is still being programmed.

This sounds like a driver bug to me. If drm_crtc_vblank_get returns
successfully, vblanks are supposed to work. If that's not the case at load
time, then something with the vblank init has gone wrong somewhere (or
fbdev is set up too early).
-Daniel

> 

> Best regards
> Thomas
> 
> > -Daniel
> > > +		drm_crtc_vblank_put(crtc);
> > > +	}
> > >   	spin_lock_irqsave(&helper->dirty_lock, flags);
> > >   	clip_copy = *clip;
> > > -- 
> > > 2.23.0
> > > 
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
> HRB 21284 (AG Nürnberg)

-- 
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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-10-08 10:01 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-12  6:42 [PATCH v2 0/3] Rate-limit shadow-FB-to-console-update to screen refresh Thomas Zimmermann
2019-09-12  6:42 ` [PATCH v2 1/3] drm/fb-helper: Synchronize dirty worker with vblank Thomas Zimmermann
2019-09-12  8:52   ` Gerd Hoffmann
2019-09-17 13:06   ` Daniel Vetter
2019-09-27  8:41     ` Thomas Zimmermann
2019-10-08 10:01       ` Daniel Vetter
2019-09-12  6:42 ` [PATCH v2 2/3] drm/mgag200: Rename constant MGAREG_Status to MGAREG_STATUS Thomas Zimmermann
2019-09-12  8:52   ` Gerd Hoffmann
2019-09-12  6:42 ` [PATCH v2 3/3] drm/mgag200: Add vblank support Thomas Zimmermann
2019-09-12  8:53   ` Gerd Hoffmann

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.