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

[Thomas Zimmermann]

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

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20191205/c6d0c7e9/attachment.sig>