* [PATCH v3 0/4] Rate-limit shadow-FB-to-console-update to screen refresh @ 2019-12-05 16:01 Thomas Zimmermann 2019-12-05 16:01 ` [PATCH v3 1/4] drm/mgag200: Create fbdev console after registering device Thomas Zimmermann ` (4 more replies) 0 siblings, 5 replies; 10+ messages in thread From: Thomas Zimmermann @ 2019-12-05 16:01 UTC (permalink / raw) To: daniel, airlied, noralf, rong.a.chen, ying.huang, ville.syrjala, sam, emil.velikov, maarten.lankhorst, mripard Cc: Thomas Zimmermann, dri-devel A full-screen memcpy() moves the console's shadow buffer to hardware; with possibly significant runtime overhead. [1] Synchronizing the screen update with VBLANK events can hopefully reduce the number of screen updates. The patchset first adds vblank support to mgag200 as the problem was initially reported for Matrox hardware. 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. In version 2 of this patchset, fbdev emulation missed the first vblank event because mgag200 initialized the fbdev console before the irq. Initializing fbdev last fixes this problem. If other drivers now start reporting a missed vblank during boot, this might be the reason. The patchset was tested by running fbdev emulation and Gnome (X11) on mgag200 HW. v3: * fbdev: hold helper->mutex * fbdev: acquire DRM master so other masters cannot interfere * mgag200: init fbdev after irq avoids missing first vblank * mgag200: trigger irq at <vdisplay> + 1 matches vblank v2: * remove locking from fbdev patch * use constants for 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 (4): drm/mgag200: Create fbdev console after registering device drm/mgag200: Rename constant MGAREG_Status to MGAREG_STATUS drm/mgag200: Add vblank support drm/fb-helper: Synchronize dirty worker with vblank drivers/gpu/drm/drm_fb_helper.c | 21 ++++++++++ drivers/gpu/drm/mgag200/mgag200_drv.c | 7 ++++ drivers/gpu/drm/mgag200/mgag200_drv.h | 1 + drivers/gpu/drm/mgag200/mgag200_main.c | 38 +++++++++++++++++- drivers/gpu/drm/mgag200/mgag200_mode.c | 54 ++++++++++++++++++++++---- drivers/gpu/drm/mgag200/mgag200_reg.h | 7 +++- 6 files changed, 119 insertions(+), 9 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 v3 1/4] drm/mgag200: Create fbdev console after registering device 2019-12-05 16:01 [PATCH v3 0/4] Rate-limit shadow-FB-to-console-update to screen refresh Thomas Zimmermann @ 2019-12-05 16:01 ` Thomas Zimmermann 2019-12-05 16:01 ` [PATCH v3 2/4] drm/mgag200: Rename constant MGAREG_Status to MGAREG_STATUS Thomas Zimmermann ` (3 subsequent siblings) 4 siblings, 0 replies; 10+ messages in thread From: Thomas Zimmermann @ 2019-12-05 16:01 UTC (permalink / raw) To: daniel, airlied, noralf, rong.a.chen, ying.huang, ville.syrjala, sam, emil.velikov, maarten.lankhorst, mripard Cc: Thomas Zimmermann, dri-devel Generic fbdev console emulation is a DRM client. For consistency with other clients, it should be created after registering the DRM device. Hence move the fbdev code to the appropriate place. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/mgag200/mgag200_drv.c | 6 ++++++ drivers/gpu/drm/mgag200/mgag200_main.c | 4 ---- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c index 30b3b827adf8..8e641b29eb01 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.c +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c @@ -74,8 +74,14 @@ static int mga_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) if (ret) goto err_mgag200_driver_unload; + ret = drm_fbdev_generic_setup(dev, 0); + if (ret) + goto err_mgag200_driver_unregister; + return 0; +err_mgag200_driver_unregister: + drm_dev_unregister(dev); err_mgag200_driver_unload: mgag200_driver_unload(dev); err_drm_dev_put: diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c index e278b6a547bd..b680cf47cbb9 100644 --- a/drivers/gpu/drm/mgag200/mgag200_main.c +++ b/drivers/gpu/drm/mgag200/mgag200_main.c @@ -181,10 +181,6 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags) dev_warn(&dev->pdev->dev, "Could not initialize cursors. Not doing hardware cursors.\n"); - r = drm_fbdev_generic_setup(mdev->dev, 0); - if (r) - goto err_modeset; - return 0; err_modeset: -- 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 v3 2/4] drm/mgag200: Rename constant MGAREG_Status to MGAREG_STATUS 2019-12-05 16:01 [PATCH v3 0/4] Rate-limit shadow-FB-to-console-update to screen refresh Thomas Zimmermann 2019-12-05 16:01 ` [PATCH v3 1/4] drm/mgag200: Create fbdev console after registering device Thomas Zimmermann @ 2019-12-05 16:01 ` Thomas Zimmermann 2019-12-05 16:01 ` [PATCH v3 3/4] drm/mgag200: Add vblank support Thomas Zimmermann ` (2 subsequent siblings) 4 siblings, 0 replies; 10+ messages in thread From: Thomas Zimmermann @ 2019-12-05 16:01 UTC (permalink / raw) To: daniel, airlied, noralf, rong.a.chen, ying.huang, ville.syrjala, sam, emil.velikov, maarten.lankhorst, mripard Cc: Thomas Zimmermann, dri-devel, Gerd Hoffmann 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 62a8e9ccb16d..9c4440d7b7b4 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 v3 3/4] drm/mgag200: Add vblank support 2019-12-05 16:01 [PATCH v3 0/4] Rate-limit shadow-FB-to-console-update to screen refresh Thomas Zimmermann 2019-12-05 16:01 ` [PATCH v3 1/4] drm/mgag200: Create fbdev console after registering device Thomas Zimmermann 2019-12-05 16:01 ` [PATCH v3 2/4] drm/mgag200: Rename constant MGAREG_Status to MGAREG_STATUS Thomas Zimmermann @ 2019-12-05 16:01 ` Thomas Zimmermann 2019-12-05 17:25 ` Sam Ravnborg 2019-12-05 16:01 ` [PATCH v3 4/4] drm/fb-helper: Synchronize dirty worker with vblank Thomas Zimmermann 2019-12-05 17:31 ` [PATCH v3 0/4] Rate-limit shadow-FB-to-console-update to screen refresh Sam Ravnborg 4 siblings, 1 reply; 10+ messages in thread From: Thomas Zimmermann @ 2019-12-05 16:01 UTC (permalink / raw) To: daniel, airlied, noralf, rong.a.chen, ying.huang, ville.syrjala, sam, emil.velikov, maarten.lankhorst, mripard Cc: Thomas Zimmermann, dri-devel, Gerd Hoffmann 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> + 1 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 syncihronization between irq handler and the rest of the driver is required. v3: * set <linecomp> to <vdisplay> + 1 to trigger at VBLANK * expand comment on linecomp 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 Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> Acked-by: Gerd Hoffmann <kraxel@redhat.com> --- 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 | 48 +++++++++++++++++++++++--- drivers/gpu/drm/mgag200/mgag200_reg.h | 5 +++ 5 files changed, 91 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c index 8e641b29eb01..15cb39c08989 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.c +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c @@ -133,6 +133,7 @@ int mgag200_driver_dumb_create(struct drm_file *file, static struct drm_driver driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET, + .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 aa32aad222c2..8af57dfe0d09 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -206,6 +206,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 b680cf47cbb9..72ebcd015fba 100644 --- a/drivers/gpu/drm/mgag200/mgag200_main.c +++ b/drivers/gpu/drm/mgag200/mgag200_main.c @@ -12,6 +12,8 @@ #include <drm/drm_crtc_helper.h> #include <drm/drm_gem_framebuffer_helper.h> +#include <drm/drm_irq.h> +#include <drm/drm_vblank.h> #include "mgag200_drv.h" @@ -181,6 +183,14 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags) dev_warn(&dev->pdev->dev, "Could not initialize cursors. Not doing hardware cursors.\n"); + r = drm_vblank_init(dev, 1); + if (r) + goto err_modeset; + + r = drm_irq_install(dev, dev->pdev->irq); + if (r) + goto err_modeset; + return 0; err_modeset: @@ -199,9 +209,39 @@ 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_cursor_fini(mdev); 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 9c4440d7b7b4..4d0b9e497149 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,15 @@ 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 use + * the VLINE interrupt instead. It triggers when the current + * linecomp has been reached. For VBLANK, this is the first + * non-visible line at the bottom of the screen. Therefore, + * keep <linecomp> in sync with <vdisplay>. + */ + linecomp = vdisplay + 1; + WREG_GFX(0, 0); WREG_GFX(1, 0); WREG_GFX(2, 0); @@ -1063,12 +1073,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 +1093,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 +1109,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 +1421,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 = mgag200_crtc_cursor_set, @@ -1418,6 +1456,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 v3 3/4] drm/mgag200: Add vblank support 2019-12-05 16:01 ` [PATCH v3 3/4] drm/mgag200: Add vblank support Thomas Zimmermann @ 2019-12-05 17:25 ` Sam Ravnborg 2019-12-05 18:28 ` Thomas Zimmermann 0 siblings, 1 reply; 10+ messages in thread From: Sam Ravnborg @ 2019-12-05 17:25 UTC (permalink / raw) To: Thomas Zimmermann Cc: Gerd Hoffmann, rong.a.chen, dri-devel, ying.huang, airlied, emil.velikov Hi Thomas. Some nits you can address when applying, if you think they shall be addressed. Sam On Thu, Dec 05, 2019 at 05:01:41PM +0100, 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> + 1 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 syncihronization between irq handler and the rest of the driver is s/syncihronization/synchronization/ > required. > > ((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 +1093,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); Use 0xFF like other code nearby? > > ext_vga[0] = 0; > ext_vga[5] = 0; > @@ -1099,7 +1109,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 +1421,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 = mgag200_crtc_cursor_set, > @@ -1418,6 +1456,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) Use BIT(5)? The bitfield name could be more readable if they included the register name. Example: #define MGA_STATUS_VLINEPEN BIT(5) #define MGA_ICLEAR_VLINECLR BIT(5) #define MGA_IEN_VLINEIEN BIT(5) Sam _______________________________________________ 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 v3 3/4] drm/mgag200: Add vblank support 2019-12-05 17:25 ` Sam Ravnborg @ 2019-12-05 18:28 ` Thomas Zimmermann 2019-12-05 18:56 ` Emil Velikov 0 siblings, 1 reply; 10+ messages in thread From: Thomas Zimmermann @ 2019-12-05 18:28 UTC (permalink / raw) To: Sam Ravnborg Cc: rong.a.chen, dri-devel, Gerd Hoffmann, ying.huang, airlied, emil.velikov [-- Attachment #1.1.1: Type: text/plain, Size: 4701 bytes --] Hi Am 05.12.19 um 18:25 schrieb Sam Ravnborg: > Hi Thomas. > > Some nits you can address when applying, if you think they shall be > addressed. > > Sam > > On Thu, Dec 05, 2019 at 05:01:41PM +0100, 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> + 1 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 syncihronization between irq handler and the rest of the driver is > s/syncihronization/synchronization/ Oh. > >> required. >> >> ((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 +1093,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); > Use 0xFF like other code nearby? The code in this file is a mixture of numbers in upper and lower case. I used lower case here as I found if more pleasant to read. Hopefully the other constants can be switched when the code is converted to atomic modesetting. > >> >> ext_vga[0] = 0; >> ext_vga[5] = 0; >> @@ -1099,7 +1109,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 +1421,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 = mgag200_crtc_cursor_set, >> @@ -1418,6 +1456,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) > Use BIT(5)? > The bitfield name could be more readable if they included the register > name. > Example: > #define MGA_STATUS_VLINEPEN BIT(5) > #define MGA_ICLEAR_VLINECLR BIT(5) > #define MGA_IEN_VLINEIEN BIT(5) Oh, good point. I wasn't aware of this macro. Best regards Thomas > > Sam > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel > -- 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: 159 bytes --] _______________________________________________ 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 v3 3/4] drm/mgag200: Add vblank support 2019-12-05 18:28 ` Thomas Zimmermann @ 2019-12-05 18:56 ` Emil Velikov 0 siblings, 0 replies; 10+ messages in thread From: Emil Velikov @ 2019-12-05 18:56 UTC (permalink / raw) To: Thomas Zimmermann Cc: rong.a.chen, ML dri-devel, Gerd Hoffmann, ying.huang, Dave Airlie, Sam Ravnborg, Emil Velikov On Thu, 5 Dec 2019 at 18:28, Thomas Zimmermann <tzimmermann@suse.de> wrote: > >> +/* Interrupt fields */ > >> +#define MGA_VLINEPEN (0x01 << 5) > >> +#define MGA_VLINECLR (0x01 << 5) > >> +#define MGA_VLINEIEN (0x01 << 5) > > Use BIT(5)? > > The bitfield name could be more readable if they included the register > > name. > > Example: > > #define MGA_STATUS_VLINEPEN BIT(5) > > #define MGA_ICLEAR_VLINECLR BIT(5) > > #define MGA_IEN_VLINEIEN BIT(5) > > Oh, good point. I wasn't aware of this macro. > While at it, please keep bitfields close to the respective registers. Don't know much about the driver to review this, for 1&2 Reviewed-by: Emil Velikov <emil.velikov@collabora.com> I'll look at 4/4 first thing tomorrow. Thanks Emil _______________________________________________ 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 v3 4/4] drm/fb-helper: Synchronize dirty worker with vblank 2019-12-05 16:01 [PATCH v3 0/4] Rate-limit shadow-FB-to-console-update to screen refresh Thomas Zimmermann ` (2 preceding siblings ...) 2019-12-05 16:01 ` [PATCH v3 3/4] drm/mgag200: Add vblank support Thomas Zimmermann @ 2019-12-05 16:01 ` Thomas Zimmermann 2019-12-09 14:11 ` Emil Velikov 2019-12-05 17:31 ` [PATCH v3 0/4] Rate-limit shadow-FB-to-console-update to screen refresh Sam Ravnborg 4 siblings, 1 reply; 10+ messages in thread From: Thomas Zimmermann @ 2019-12-05 16:01 UTC (permalink / raw) To: daniel, airlied, noralf, rong.a.chen, ying.huang, ville.syrjala, sam, emil.velikov, maarten.lankhorst, mripard Cc: Thomas Zimmermann, dri-devel Before updating the display from the console's shadow buffer, the dirty worker now waits for a vblank. This allows several screen updates to pile up and acts as a rate limiter. If a DRM master is present, it could interfere with the vblank. Don't wait in this case. v3: * add back helper->lock * acquire DRM master status while waiting for vblank 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 | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index fb9bff0f4581..ba20ad92fb90 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -404,8 +404,29 @@ 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 to vblank. If there's a DRM master + * present, it could interfere while we're waiting for the vblank + * event. Don't wait in this case. + */ + mutex_lock(&helper->lock); + if (!drm_master_internal_acquire(helper->dev)) { + goto unlock; + } + 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); + } + drm_master_internal_release(helper->dev); +unlock: + mutex_unlock(&helper->lock); 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
* Re: [PATCH v3 4/4] drm/fb-helper: Synchronize dirty worker with vblank 2019-12-05 16:01 ` [PATCH v3 4/4] drm/fb-helper: Synchronize dirty worker with vblank Thomas Zimmermann @ 2019-12-09 14:11 ` Emil Velikov 0 siblings, 0 replies; 10+ messages in thread From: Emil Velikov @ 2019-12-09 14:11 UTC (permalink / raw) To: Thomas Zimmermann Cc: rong.a.chen, ML dri-devel, ying.huang, Dave Airlie, Sam Ravnborg, Emil Velikov Hi Thomas, On Thu, 5 Dec 2019 at 16:01, Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Before updating the display from the console's shadow buffer, the dirty > worker now waits for a vblank. This allows several screen updates to pile > up and acts as a rate limiter. If a DRM master is present, it could > interfere with the vblank. Don't wait in this case. > > v3: > * add back helper->lock > * acquire DRM master status while waiting for vblank > 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 | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index fb9bff0f4581..ba20ad92fb90 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -404,8 +404,29 @@ 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 to vblank. If there's a DRM master > + * present, it could interfere while we're waiting for the vblank > + * event. Don't wait in this case. > + */ > + mutex_lock(&helper->lock); > + if (!drm_master_internal_acquire(helper->dev)) { > + goto unlock; > + } > + 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); > + } > + drm_master_internal_release(helper->dev); > +unlock: > + mutex_unlock(&helper->lock); > This hunk seems surprisingly similar to the FBIO_WAITFORVSYNC code in drm_fb_helper_ioctl(). Modulo the really neat comment, which answers the "why do we use modeset[0]" instead of any other (or all). Can I suggest fleshing that function out (alongside all the locking) into a helper and reusing it here. Pretty sure we can live with the (very unlikely) EBUSY -> ENOTTY change, in drm_fb_helper_ioctl() HTH Emil _______________________________________________ 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 v3 0/4] Rate-limit shadow-FB-to-console-update to screen refresh 2019-12-05 16:01 [PATCH v3 0/4] Rate-limit shadow-FB-to-console-update to screen refresh Thomas Zimmermann ` (3 preceding siblings ...) 2019-12-05 16:01 ` [PATCH v3 4/4] drm/fb-helper: Synchronize dirty worker with vblank Thomas Zimmermann @ 2019-12-05 17:31 ` Sam Ravnborg 4 siblings, 0 replies; 10+ messages in thread From: Sam Ravnborg @ 2019-12-05 17:31 UTC (permalink / raw) To: Thomas Zimmermann Cc: rong.a.chen, dri-devel, ying.huang, airlied, emil.velikov Hi Thomas. On Thu, Dec 05, 2019 at 05:01:38PM +0100, Thomas Zimmermann wrote: > A full-screen memcpy() moves the console's shadow buffer to hardware; with > possibly significant runtime overhead. [1] Synchronizing the screen update > with VBLANK events can hopefully reduce the number of screen updates. > > The patchset first adds vblank support to mgag200 as the problem was > initially reported for Matrox hardware. > > 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. > > In version 2 of this patchset, fbdev emulation missed the first vblank > event because mgag200 initialized the fbdev console before the irq. > Initializing fbdev last fixes this problem. If other drivers now start > reporting a missed vblank during boot, this might be the reason. > > The patchset was tested by running fbdev emulation and Gnome (X11) on > mgag200 HW. > > v3: > * fbdev: hold helper->mutex > * fbdev: acquire DRM master so other masters cannot interfere > * mgag200: init fbdev after irq avoids missing first vblank > * mgag200: trigger irq at <vdisplay> + 1 matches vblank > v2: > * remove locking from fbdev patch > * use constants for 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 (4): > drm/mgag200: Create fbdev console after registering device > drm/mgag200: Rename constant MGAREG_Status to MGAREG_STATUS > drm/mgag200: Add vblank support > drm/fb-helper: Synchronize dirty worker with vblank Whole series is: Acked-by: Sam Ravnborg <sam@ravnborg.org> The code all looked good, but I am not familiar enough with the code to say I reviewed it all. A few nits in one patch sent in a separate mail. Sam _______________________________________________ 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-12-09 14:13 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-05 16:01 [PATCH v3 0/4] Rate-limit shadow-FB-to-console-update to screen refresh Thomas Zimmermann 2019-12-05 16:01 ` [PATCH v3 1/4] drm/mgag200: Create fbdev console after registering device Thomas Zimmermann 2019-12-05 16:01 ` [PATCH v3 2/4] drm/mgag200: Rename constant MGAREG_Status to MGAREG_STATUS Thomas Zimmermann 2019-12-05 16:01 ` [PATCH v3 3/4] drm/mgag200: Add vblank support Thomas Zimmermann 2019-12-05 17:25 ` Sam Ravnborg 2019-12-05 18:28 ` Thomas Zimmermann 2019-12-05 18:56 ` Emil Velikov 2019-12-05 16:01 ` [PATCH v3 4/4] drm/fb-helper: Synchronize dirty worker with vblank Thomas Zimmermann 2019-12-09 14:11 ` Emil Velikov 2019-12-05 17:31 ` [PATCH v3 0/4] Rate-limit shadow-FB-to-console-update to screen refresh Sam Ravnborg
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.