| Message ID | 20190909140633.31260-3-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rate-limit shadow-FB-to-console-update to screen refresh | expand |
On Mon, Sep 09, 2019 at 04:06:33PM +0200, Thomas Zimmermann wrote: > Support for vblank requires VSYNC to signal an interrupt, which is broken > on Matrox chipsets. The workaround that is 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 same time > when VSYNC 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. Looks good overall, some minor nits ... > +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, iclear; > + > + status = RREG32(0x1e14); > + > + if (status & 0x00000020) { /* test <vlinepen> */ > + drm_for_each_crtc(crtc, dev) { > + drm_crtc_handle_vblank(crtc); > + } > + iclear = RREG32(0x1e18); > + iclear |= 0x00000020; /* set <vlineiclr> */ #define for this would be good (you also don't need the comment then). > + /* The VSYNC interrupt is broken on Matrox chipsets. We use Codestyle. "/*" should be on a separate line. > +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(0x1e1c); > + ien &= 0xffffffdf; /* clear <vlineien> */ That is typically written as value &= ~(bit); cheers, Gerd
On Mon, Sep 09, 2019 at 04:06:33PM +0200, Thomas Zimmermann wrote: > Support for vblank requires VSYNC to signal an interrupt, which is broken > on Matrox chipsets. I don't remember there being anything wrong with the vsync interrupt. What makes you think it's broken? > The workaround that is 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 same time > when VSYNC begins. You're programming it to fire at start of vblank, not start of vsync. > > 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> > --- > drivers/gpu/drm/mgag200/mgag200_drv.c | 1 + > drivers/gpu/drm/mgag200/mgag200_drv.h | 1 + > drivers/gpu/drm/mgag200/mgag200_main.c | 33 ++++++++++++++++++++ > drivers/gpu/drm/mgag200/mgag200_mode.c | 42 +++++++++++++++++++++++--- > 4 files changed, 73 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..5941607796e8 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,31 @@ 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, iclear; > + > + status = RREG32(0x1e14); > + > + if (status & 0x00000020) { /* test <vlinepen> */ > + drm_for_each_crtc(crtc, dev) { > + drm_crtc_handle_vblank(crtc); > + } A bit odd way to write that but as long this driver doesn't support crtc2 it should be fine. > + iclear = RREG32(0x1e18); > + iclear |= 0x00000020; /* set <vlineiclr> */ > + WREG32(0x1e18, iclear); > + return IRQ_HANDLED; > + } > + > + return IRQ_NONE; > +}; > diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c > index 5e778b5f1a10..ffe5f15d0a7d 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,13 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc, > vsyncend = mode->vsync_end - 1; > vtotal = mode->vtotal - 2; > > + /* The VSYNC interrupt is broken on Matrox chipsets. We use > + * the VLINE interrupt instead. It triggers when the current > + * linecomp has been reached. Therefore keep <linecomp> in > + * sync with <vdisplay>. > + */ > + linecomp = vdisplay; I have an odd recollection that you want vdisplay+1 here if you want the interrupt to fire at the correct time. Sine linecomp also resets the memory address counter to 0 you should probably see one bogus line at the bottom of the screen if my recollection of that +1 is correct. But maybe my memory is wrong. > + > WREG_GFX(0, 0); > WREG_GFX(1, 0); > WREG_GFX(2, 0); > @@ -1063,12 +1071,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 +1091,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 +1107,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 +1419,30 @@ 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 ien; > + > + ien = RREG32(0x1e1c); MGAREG_IEN? > + ien |= 0x00000020; /* set <vlineien> */ > + WREG32(0x1e1c, 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(0x1e1c); > + ien &= 0xffffffdf; /* clear <vlineien> */ > + WREG32(0x1e1c, 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 +1450,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 = { > -- > 2.23.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi thanks for the feedback. Am 10.09.19 um 16:01 schrieb Ville Syrjälä: > On Mon, Sep 09, 2019 at 04:06:33PM +0200, Thomas Zimmermann wrote: >> Support for vblank requires VSYNC to signal an interrupt, which is broken >> on Matrox chipsets. > > I don't remember there being anything wrong with the vsync interrupt. > What makes you think it's broken? I tested vsync with the same code as in this patch, with extra spin locks to protect against concurrent register access (vsync uses crtc11h). A few things constantly went wrong: I never received it when setting linecomp to vdisplay. However it fired when linecomp was ~0. Then I regularly missed the first interrupt (but only the first), and occasionally my test system crashed when I entered commands on the serial terminal. The vsync irq is maybe not entirely broken, but appears to be much harder to get right. >> The workaround that is 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 same time >> when VSYNC begins. > > You're programming it to fire at start of vblank, not start of vsync. Ah, sorry for messing up terminology; will be fixed. No matter what's wrong with the vsync irq: there's no vblank irq, so using linecomp should be the way to go in any case. > >> >> 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> >> --- >> drivers/gpu/drm/mgag200/mgag200_drv.c | 1 + >> drivers/gpu/drm/mgag200/mgag200_drv.h | 1 + >> drivers/gpu/drm/mgag200/mgag200_main.c | 33 ++++++++++++++++++++ >> drivers/gpu/drm/mgag200/mgag200_mode.c | 42 +++++++++++++++++++++++--- >> 4 files changed, 73 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..5941607796e8 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,31 @@ 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, iclear; >> + >> + status = RREG32(0x1e14); >> + >> + if (status & 0x00000020) { /* test <vlinepen> */ >> + drm_for_each_crtc(crtc, dev) { >> + drm_crtc_handle_vblank(crtc); >> + } > > A bit odd way to write that but as long this driver doesn't support > crtc2 it should be fine. > >> + iclear = RREG32(0x1e18); >> + iclear |= 0x00000020; /* set <vlineiclr> */ >> + WREG32(0x1e18, iclear); >> + return IRQ_HANDLED; >> + } >> + >> + return IRQ_NONE; >> +}; >> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c >> index 5e778b5f1a10..ffe5f15d0a7d 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,13 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc, >> vsyncend = mode->vsync_end - 1; >> vtotal = mode->vtotal - 2; >> >> + /* The VSYNC interrupt is broken on Matrox chipsets. We use >> + * the VLINE interrupt instead. It triggers when the current >> + * linecomp has been reached. Therefore keep <linecomp> in >> + * sync with <vdisplay>. >> + */ >> + linecomp = vdisplay; > > I have an odd recollection that you want vdisplay+1 here if you > want the interrupt to fire at the correct time. > > Sine linecomp also resets the memory address counter to 0 you > should probably see one bogus line at the bottom of the screen > if my recollection of that +1 is correct. It works reliably on my test system, but I'll double-check against existing drivers. > But maybe my memory is wrong. > >> + >> WREG_GFX(0, 0); >> WREG_GFX(1, 0); >> WREG_GFX(2, 0); >> @@ -1063,12 +1071,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 +1091,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 +1107,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 +1419,30 @@ 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 ien; >> + >> + ien = RREG32(0x1e1c); > > MGAREG_IEN? I'll go through the changes and use constants everywhere. Best regards Thomas >> + ien |= 0x00000020; /* set <vlineien> */ >> + WREG32(0x1e1c, 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(0x1e1c); >> + ien &= 0xffffffdf; /* clear <vlineien> */ >> + WREG32(0x1e1c, 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 +1450,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 = { >> -- >> 2.23.0 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >
On Mon, Sep 09, 2019 at 04:06:33PM +0200, Thomas Zimmermann wrote: > Support for vblank requires VSYNC to signal an interrupt, which is broken > on Matrox chipsets. The workaround that is 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 same time > when VSYNC 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> > --- > drivers/gpu/drm/mgag200/mgag200_drv.c | 1 + > drivers/gpu/drm/mgag200/mgag200_drv.h | 1 + > drivers/gpu/drm/mgag200/mgag200_main.c | 33 ++++++++++++++++++++ > drivers/gpu/drm/mgag200/mgag200_mode.c | 42 +++++++++++++++++++++++--- > 4 files changed, 73 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..5941607796e8 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,31 @@ 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, iclear; > + > + status = RREG32(0x1e14); > + > + if (status & 0x00000020) { /* test <vlinepen> */ On further inspection this looks a bit iffy. IIRC the status bits aren't masked out by IEN, so I would do something like this: irq_handler() { status = read(STATUS) & read(IEN): if (status == 0) return IRQ_NONE; write(ICLEAR, status); drm_handle_vblank(); return IRQ_HANDLED; } vblank_enable() { write(ICLEAR, vline); write(IEN, vline); } vblank_disable() { write(IEN, 0); } Otherwise you maybe try to handle a stale interrupt when enabling the vblank irq, and also could accidentally handle bogus interrupts from other devices on the same irq line. And probably clear IEN on before registering the irq handler for good measure. > + drm_for_each_crtc(crtc, dev) { > + drm_crtc_handle_vblank(crtc); > + } > + iclear = RREG32(0x1e18); > + iclear |= 0x00000020; /* set <vlineiclr> */ > + WREG32(0x1e18, iclear); > + return IRQ_HANDLED; > + } > + > + return IRQ_NONE; > +};
Hi Am 10.09.19 um 16:01 schrieb Ville Syrjälä: > On Mon, Sep 09, 2019 at 04:06:33PM +0200, Thomas Zimmermann wrote: >> Support for vblank requires VSYNC to signal an interrupt, which is broken >> on Matrox chipsets. > > I don't remember there being anything wrong with the vsync interrupt. > What makes you think it's broken? > >> The workaround that is 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 same time >> when VSYNC begins. > > You're programming it to fire at start of vblank, not start of vsync. > >> >> 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> >> --- >> drivers/gpu/drm/mgag200/mgag200_drv.c | 1 + >> drivers/gpu/drm/mgag200/mgag200_drv.h | 1 + >> drivers/gpu/drm/mgag200/mgag200_main.c | 33 ++++++++++++++++++++ >> drivers/gpu/drm/mgag200/mgag200_mode.c | 42 +++++++++++++++++++++++--- >> 4 files changed, 73 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..5941607796e8 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,31 @@ 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, iclear; >> + >> + status = RREG32(0x1e14); >> + >> + if (status & 0x00000020) { /* test <vlinepen> */ >> + drm_for_each_crtc(crtc, dev) { >> + drm_crtc_handle_vblank(crtc); >> + } > > A bit odd way to write that but as long this driver doesn't support > crtc2 it should be fine. > >> + iclear = RREG32(0x1e18); >> + iclear |= 0x00000020; /* set <vlineiclr> */ >> + WREG32(0x1e18, iclear); >> + return IRQ_HANDLED; >> + } >> + >> + return IRQ_NONE; >> +}; >> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c >> index 5e778b5f1a10..ffe5f15d0a7d 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,13 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc, >> vsyncend = mode->vsync_end - 1; >> vtotal = mode->vtotal - 2; >> >> + /* The VSYNC interrupt is broken on Matrox chipsets. We use >> + * the VLINE interrupt instead. It triggers when the current >> + * linecomp has been reached. Therefore keep <linecomp> in >> + * sync with <vdisplay>. >> + */ >> + linecomp = vdisplay; > > I have an odd recollection that you want vdisplay+1 here if you > want the interrupt to fire at the correct time. I double-checked and found that vdisplay is the value originally used by this driver, by the fbdev driver and by Xorg's mga driver. Best regards Thomas > Sine linecomp also resets the memory address counter to 0 you > should probably see one bogus line at the bottom of the screen > if my recollection of that +1 is correct. > > But maybe my memory is wrong. > >> + >> WREG_GFX(0, 0); >> WREG_GFX(1, 0); >> WREG_GFX(2, 0); >> @@ -1063,12 +1071,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 +1091,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 +1107,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 +1419,30 @@ 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 ien; >> + >> + ien = RREG32(0x1e1c); > > MGAREG_IEN? > >> + ien |= 0x00000020; /* set <vlineien> */ >> + WREG32(0x1e1c, 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(0x1e1c); >> + ien &= 0xffffffdf; /* clear <vlineien> */ >> + WREG32(0x1e1c, 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 +1450,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 = { >> -- >> 2.23.0 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >
On Wed, Sep 11, 2019 at 05:08:45PM +0200, Thomas Zimmermann wrote: > Hi > > Am 10.09.19 um 16:01 schrieb Ville Syrjälä: > > On Mon, Sep 09, 2019 at 04:06:33PM +0200, Thomas Zimmermann wrote: > >> Support for vblank requires VSYNC to signal an interrupt, which is broken > >> on Matrox chipsets. > > > > I don't remember there being anything wrong with the vsync interrupt. > > What makes you think it's broken? > > > >> The workaround that is 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 same time > >> when VSYNC begins. > > > > You're programming it to fire at start of vblank, not start of vsync. > > > >> > >> 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> > >> --- > >> drivers/gpu/drm/mgag200/mgag200_drv.c | 1 + > >> drivers/gpu/drm/mgag200/mgag200_drv.h | 1 + > >> drivers/gpu/drm/mgag200/mgag200_main.c | 33 ++++++++++++++++++++ > >> drivers/gpu/drm/mgag200/mgag200_mode.c | 42 +++++++++++++++++++++++--- > >> 4 files changed, 73 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..5941607796e8 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,31 @@ 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, iclear; > >> + > >> + status = RREG32(0x1e14); > >> + > >> + if (status & 0x00000020) { /* test <vlinepen> */ > >> + drm_for_each_crtc(crtc, dev) { > >> + drm_crtc_handle_vblank(crtc); > >> + } > > > > A bit odd way to write that but as long this driver doesn't support > > crtc2 it should be fine. > > > >> + iclear = RREG32(0x1e18); > >> + iclear |= 0x00000020; /* set <vlineiclr> */ > >> + WREG32(0x1e18, iclear); > >> + return IRQ_HANDLED; > >> + } > >> + > >> + return IRQ_NONE; > >> +}; > >> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c > >> index 5e778b5f1a10..ffe5f15d0a7d 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,13 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc, > >> vsyncend = mode->vsync_end - 1; > >> vtotal = mode->vtotal - 2; > >> > >> + /* The VSYNC interrupt is broken on Matrox chipsets. We use > >> + * the VLINE interrupt instead. It triggers when the current > >> + * linecomp has been reached. Therefore keep <linecomp> in > >> + * sync with <vdisplay>. > >> + */ > >> + linecomp = vdisplay; > > > > I have an odd recollection that you want vdisplay+1 here if you > > want the interrupt to fire at the correct time. > > I double-checked and found that vdisplay is the value originally used by > this driver, by the fbdev driver and by Xorg's mga driver. Sure, but what does the hardware actually want?
Hi Am 11.09.19 um 17:21 schrieb Ville Syrjälä: > On Wed, Sep 11, 2019 at 05:08:45PM +0200, Thomas Zimmermann wrote: >> Hi >> >> Am 10.09.19 um 16:01 schrieb Ville Syrjälä: >>> On Mon, Sep 09, 2019 at 04:06:33PM +0200, Thomas Zimmermann wrote: >>>> Support for vblank requires VSYNC to signal an interrupt, which is broken >>>> on Matrox chipsets. >>> >>> I don't remember there being anything wrong with the vsync interrupt. >>> What makes you think it's broken? >>> >>>> The workaround that is 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 same time >>>> when VSYNC begins. >>> >>> You're programming it to fire at start of vblank, not start of vsync. >>> >>>> >>>> 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> >>>> --- >>>> drivers/gpu/drm/mgag200/mgag200_drv.c | 1 + >>>> drivers/gpu/drm/mgag200/mgag200_drv.h | 1 + >>>> drivers/gpu/drm/mgag200/mgag200_main.c | 33 ++++++++++++++++++++ >>>> drivers/gpu/drm/mgag200/mgag200_mode.c | 42 +++++++++++++++++++++++--- >>>> 4 files changed, 73 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..5941607796e8 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,31 @@ 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, iclear; >>>> + >>>> + status = RREG32(0x1e14); >>>> + >>>> + if (status & 0x00000020) { /* test <vlinepen> */ >>>> + drm_for_each_crtc(crtc, dev) { >>>> + drm_crtc_handle_vblank(crtc); >>>> + } >>> >>> A bit odd way to write that but as long this driver doesn't support >>> crtc2 it should be fine. >>> >>>> + iclear = RREG32(0x1e18); >>>> + iclear |= 0x00000020; /* set <vlineiclr> */ >>>> + WREG32(0x1e18, iclear); >>>> + return IRQ_HANDLED; >>>> + } >>>> + >>>> + return IRQ_NONE; >>>> +}; >>>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c >>>> index 5e778b5f1a10..ffe5f15d0a7d 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,13 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc, >>>> vsyncend = mode->vsync_end - 1; >>>> vtotal = mode->vtotal - 2; >>>> >>>> + /* The VSYNC interrupt is broken on Matrox chipsets. We use >>>> + * the VLINE interrupt instead. It triggers when the current >>>> + * linecomp has been reached. Therefore keep <linecomp> in >>>> + * sync with <vdisplay>. >>>> + */ >>>> + linecomp = vdisplay; >>> >>> I have an odd recollection that you want vdisplay+1 here if you >>> want the interrupt to fire at the correct time. >> >> I double-checked and found that vdisplay is the value originally used by >> this driver, by the fbdev driver and by Xorg's mga driver. > > Sure, but what does the hardware actually want? > I set linecomp to vdisplay+1 and got a DRM warning about missing a vblank event. Best regards Thomas
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..5941607796e8 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,31 @@ 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, iclear; + + status = RREG32(0x1e14); + + if (status & 0x00000020) { /* test <vlinepen> */ + drm_for_each_crtc(crtc, dev) { + drm_crtc_handle_vblank(crtc); + } + iclear = RREG32(0x1e18); + iclear |= 0x00000020; /* set <vlineiclr> */ + WREG32(0x1e18, iclear); + return IRQ_HANDLED; + } + + return IRQ_NONE; +}; diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 5e778b5f1a10..ffe5f15d0a7d 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,13 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc, vsyncend = mode->vsync_end - 1; vtotal = mode->vtotal - 2; + /* The VSYNC interrupt is broken on Matrox chipsets. We 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 +1071,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 +1091,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 +1107,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 +1419,30 @@ 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 ien; + + ien = RREG32(0x1e1c); + ien |= 0x00000020; /* set <vlineien> */ + WREG32(0x1e1c, 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(0x1e1c); + ien &= 0xffffffdf; /* clear <vlineien> */ + WREG32(0x1e1c, 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 +1450,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 = {
Support for vblank requires VSYNC to signal an interrupt, which is broken on Matrox chipsets. The workaround that is 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 same time when VSYNC 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> --- drivers/gpu/drm/mgag200/mgag200_drv.c | 1 + drivers/gpu/drm/mgag200/mgag200_drv.h | 1 + drivers/gpu/drm/mgag200/mgag200_main.c | 33 ++++++++++++++++++++ drivers/gpu/drm/mgag200/mgag200_mode.c | 42 +++++++++++++++++++++++--- 4 files changed, 73 insertions(+), 4 deletions(-)