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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
