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) |
((vdisplay & 0x200) >> 3));
((linecomp & 0x200) >> 3));@@ -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);@@ -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