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@xxxxxxxxxxxxxxxxxxxxx
> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
