[PATCH v3 3/4] drm/mgag200: Add vblank support

[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/

> 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