| Message ID | 20191205160142.3588-4-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rate-limit shadow-FB-to-console-update to screen refresh | expand |
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
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 >
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
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)