| Message ID | 20200429143238.10115-6-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/mgag200: Convert to atomic modesetting | expand |
Hi Thomas, On Wed, Apr 29, 2020 at 04:32:26PM +0200, Thomas Zimmermann wrote: > All register names and fields are now named according to the > MGA programming manuals. The function doesn't need the CRTC, so > callers pass in the device structure directly. The logging now > uses device-specific macros. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/mgag200/mgag200_drv.h | 5 ++ > drivers/gpu/drm/mgag200/mgag200_mode.c | 82 +++++++++++++++----------- > 2 files changed, 53 insertions(+), 34 deletions(-) > > diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h > index 4403145e3593c..9b957d9fc7e04 100644 > --- a/drivers/gpu/drm/mgag200/mgag200_drv.h > +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h > @@ -61,6 +61,11 @@ > WREG8(MGAREG_CRTC_DATA, v); \ > } while (0) \ > > +#define RREG_ECRT(reg, v) \ > + do { \ > + WREG8(MGAREG_CRTCEXT_INDEX, reg); \ > + v = RREG8(MGAREG_CRTCEXT_DATA); \ > + } while (0) \ > > #define WREG_ECRT(reg, v) \ > do { \ > diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c > index 3d894b37a0812..b16a73c8617d6 100644 > --- a/drivers/gpu/drm/mgag200/mgag200_mode.c > +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c > @@ -819,49 +819,53 @@ static void mga_g200wb_commit(struct drm_crtc *crtc) > } > > /* > - This is how the framebuffer base address is stored in g200 cards: > - * Assume @offset is the gpu_addr variable of the framebuffer object > - * Then addr is the number of _pixels_ (not bytes) from the start of > - VRAM to the first pixel we want to display. (divided by 2 for 32bit > - framebuffers) > - * addr is stored in the CRTCEXT0, CRTCC and CRTCD registers > - addr<20> -> CRTCEXT0<6> > - addr<19-16> -> CRTCEXT0<3-0> > - addr<15-8> -> CRTCC<7-0> > - addr<7-0> -> CRTCD<7-0> > - CRTCEXT0 has to be programmed last to trigger an update and make the > - new addr variable take effect. > + * This is how the framebuffer base address is stored in g200 cards: > + * * Assume @offset is the gpu_addr variable of the framebuffer object > + * * Then addr is the number of _pixels_ (not bytes) from the start of > + * VRAM to the first pixel we want to display. (divided by 2 for 32bit > + * framebuffers) > + * * addr is stored in the CRTCEXT0, CRTCC and CRTCD registers > + * addr<20> -> CRTCEXT0<6> > + * addr<19-16> -> CRTCEXT0<3-0> > + * addr<15-8> -> CRTCC<7-0> > + * addr<7-0> -> CRTCD<7-0> > + * > + * CRTCEXT0 has to be programmed last to trigger an update and make the > + * new addr variable take effect. > */ > -static void mga_set_start_address(struct drm_crtc *crtc, unsigned offset) > +static void mgag200_set_startadd(struct mga_device *mdev, > + unsigned long offset) > { > - struct mga_device *mdev = crtc->dev->dev_private; > - u32 addr; > - int count; > - u8 crtcext0; > + struct drm_device *dev = mdev->dev; > + uint32_t startadd; > + uint8_t crtcc, crtcd, crtcext0; > > - while (RREG8(0x1fda) & 0x08); > - while (!(RREG8(0x1fda) & 0x08)); > + startadd = offset / 8; > > - count = RREG8(MGAREG_VCOUNT) + 2; > - while (RREG8(MGAREG_VCOUNT) < count); > - > - WREG8(MGAREG_CRTCEXT_INDEX, 0); > - crtcext0 = RREG8(MGAREG_CRTCEXT_DATA); > - crtcext0 &= 0xB0; > - addr = offset / 8; > - /* Can't store addresses any higher than that... > - but we also don't have more than 16MB of memory, so it should be fine. */ > - WARN_ON(addr > 0x1fffff); > - crtcext0 |= (!!(addr & (1<<20)))<<6; > - WREG_CRT(0x0d, (u8)(addr & 0xff)); > - WREG_CRT(0x0c, (u8)(addr >> 8) & 0xff); > - WREG_ECRT(0x0, ((u8)(addr >> 16) & 0xf) | crtcext0); > + /* > + * Can't store addresses any higher than that, but we also > + * don't have more than 16MB of memory, so it should be fine. > + */ > + drm_WARN_ON(dev, startadd > 0x1fffff); > + > + RREG_ECRT(0x00, crtcext0); > + > + crtcc = (startadd >> 8) & 0xff; > + crtcd = startadd & 0xff; > + crtcext0 &= 0xb0; > + crtcext0 |= ((startadd >> 14) & BIT(6)) | It is not so obvious that the value of bit 20 is stored in bit 6 here. Maybe: crtcext0 |= ((startadd & BIT(20) >> 14) | I would find the above easier to parse. > + ((startadd >> 16) & 0x0f); > + > + WREG_CRT(0x0c, crtcc); > + WREG_CRT(0x0d, crtcd); > + WREG_ECRT(0x00, crtcext0); > } > > static int mga_crtc_do_set_base(struct drm_crtc *crtc, > struct drm_framebuffer *fb, > int x, int y, int atomic) > { > + struct mga_device *mdev = crtc->dev->dev_private; Could you use a crtc_to_mdev() macro here. So we avoid adding new users of dev_private? > struct drm_gem_vram_object *gbo; > int ret; > s64 gpu_addr; Make this unsigned long and.. > @@ -882,7 +886,7 @@ static int mga_crtc_do_set_base(struct drm_crtc *crtc, > goto err_drm_gem_vram_unpin; > } > > - mga_set_start_address(crtc, (u32)gpu_addr); > + mgag200_set_startadd(mdev, (unsigned long)gpu_addr); drop this cast. > > return 0; > > @@ -894,6 +898,16 @@ static int mga_crtc_do_set_base(struct drm_crtc *crtc, > static int mga_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, > struct drm_framebuffer *old_fb) > { > + struct drm_device *dev = crtc->dev; > + struct mga_device *mdev = dev->dev_private; > + unsigned int count; > + > + while (RREG8(0x1fda) & 0x08) { } > + while (!(RREG8(0x1fda) & 0x08)) { } > + > + count = RREG8(MGAREG_VCOUNT) + 2; > + while (RREG8(MGAREG_VCOUNT) < count) { } > + > return mga_crtc_do_set_base(crtc, old_fb, x, y, 0); > } I do not really see why this code was lifter two functions up. Before is was deep in mga_set_start_address(), now it is in mga_crtc_mode_set_base(). Puzzeled? Sam > > -- > 2.26.0
Hi Am 29.04.20 um 20:20 schrieb Sam Ravnborg: > Hi Thomas, > > On Wed, Apr 29, 2020 at 04:32:26PM +0200, Thomas Zimmermann wrote: >> All register names and fields are now named according to the >> MGA programming manuals. The function doesn't need the CRTC, so >> callers pass in the device structure directly. The logging now >> uses device-specific macros. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/gpu/drm/mgag200/mgag200_drv.h | 5 ++ >> drivers/gpu/drm/mgag200/mgag200_mode.c | 82 +++++++++++++++----------- >> 2 files changed, 53 insertions(+), 34 deletions(-) >> >> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h >> index 4403145e3593c..9b957d9fc7e04 100644 >> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h >> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h >> @@ -61,6 +61,11 @@ >> WREG8(MGAREG_CRTC_DATA, v); \ >> } while (0) \ >> >> +#define RREG_ECRT(reg, v) \ >> + do { \ >> + WREG8(MGAREG_CRTCEXT_INDEX, reg); \ >> + v = RREG8(MGAREG_CRTCEXT_DATA); \ >> + } while (0) \ >> >> #define WREG_ECRT(reg, v) \ >> do { \ >> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c >> index 3d894b37a0812..b16a73c8617d6 100644 >> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c >> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c >> @@ -819,49 +819,53 @@ static void mga_g200wb_commit(struct drm_crtc *crtc) >> } >> >> /* >> - This is how the framebuffer base address is stored in g200 cards: >> - * Assume @offset is the gpu_addr variable of the framebuffer object >> - * Then addr is the number of _pixels_ (not bytes) from the start of >> - VRAM to the first pixel we want to display. (divided by 2 for 32bit >> - framebuffers) >> - * addr is stored in the CRTCEXT0, CRTCC and CRTCD registers >> - addr<20> -> CRTCEXT0<6> >> - addr<19-16> -> CRTCEXT0<3-0> >> - addr<15-8> -> CRTCC<7-0> >> - addr<7-0> -> CRTCD<7-0> >> - CRTCEXT0 has to be programmed last to trigger an update and make the >> - new addr variable take effect. >> + * This is how the framebuffer base address is stored in g200 cards: >> + * * Assume @offset is the gpu_addr variable of the framebuffer object >> + * * Then addr is the number of _pixels_ (not bytes) from the start of >> + * VRAM to the first pixel we want to display. (divided by 2 for 32bit >> + * framebuffers) >> + * * addr is stored in the CRTCEXT0, CRTCC and CRTCD registers >> + * addr<20> -> CRTCEXT0<6> >> + * addr<19-16> -> CRTCEXT0<3-0> >> + * addr<15-8> -> CRTCC<7-0> >> + * addr<7-0> -> CRTCD<7-0> >> + * >> + * CRTCEXT0 has to be programmed last to trigger an update and make the >> + * new addr variable take effect. >> */ >> -static void mga_set_start_address(struct drm_crtc *crtc, unsigned offset) >> +static void mgag200_set_startadd(struct mga_device *mdev, >> + unsigned long offset) >> { >> - struct mga_device *mdev = crtc->dev->dev_private; >> - u32 addr; >> - int count; >> - u8 crtcext0; >> + struct drm_device *dev = mdev->dev; >> + uint32_t startadd; >> + uint8_t crtcc, crtcd, crtcext0; >> >> - while (RREG8(0x1fda) & 0x08); >> - while (!(RREG8(0x1fda) & 0x08)); >> + startadd = offset / 8; >> >> - count = RREG8(MGAREG_VCOUNT) + 2; >> - while (RREG8(MGAREG_VCOUNT) < count); >> - >> - WREG8(MGAREG_CRTCEXT_INDEX, 0); >> - crtcext0 = RREG8(MGAREG_CRTCEXT_DATA); >> - crtcext0 &= 0xB0; >> - addr = offset / 8; >> - /* Can't store addresses any higher than that... >> - but we also don't have more than 16MB of memory, so it should be fine. */ >> - WARN_ON(addr > 0x1fffff); >> - crtcext0 |= (!!(addr & (1<<20)))<<6; >> - WREG_CRT(0x0d, (u8)(addr & 0xff)); >> - WREG_CRT(0x0c, (u8)(addr >> 8) & 0xff); >> - WREG_ECRT(0x0, ((u8)(addr >> 16) & 0xf) | crtcext0); >> + /* >> + * Can't store addresses any higher than that, but we also >> + * don't have more than 16MB of memory, so it should be fine. >> + */ >> + drm_WARN_ON(dev, startadd > 0x1fffff); >> + >> + RREG_ECRT(0x00, crtcext0); >> + >> + crtcc = (startadd >> 8) & 0xff; >> + crtcd = startadd & 0xff; >> + crtcext0 &= 0xb0; > >> + crtcext0 |= ((startadd >> 14) & BIT(6)) | > It is not so obvious that the value of bit 20 is stored in bit 6 here. > > Maybe: > crtcext0 |= ((startadd & BIT(20) >> 14) | > > I would find the above easier to parse. Ok. I can change that. > >> + ((startadd >> 16) & 0x0f); > >> + >> + WREG_CRT(0x0c, crtcc); >> + WREG_CRT(0x0d, crtcd); >> + WREG_ECRT(0x00, crtcext0); >> } >> >> static int mga_crtc_do_set_base(struct drm_crtc *crtc, >> struct drm_framebuffer *fb, >> int x, int y, int atomic) >> { >> + struct mga_device *mdev = crtc->dev->dev_private; > Could you use a crtc_to_mdev() macro here. > So we avoid adding new users of dev_private? I introduce such a macro in a later patch. I guess I'll add a separate patch for the macro and the conversion of all these dev_private references. > >> struct drm_gem_vram_object *gbo; >> int ret; >> s64 gpu_addr; > Make this unsigned long and.. The function that returns gpu_addr can fail (but shouldn't) with a negative error. That's why it's signed. > >> @@ -882,7 +886,7 @@ static int mga_crtc_do_set_base(struct drm_crtc *crtc, >> goto err_drm_gem_vram_unpin; >> } >> >> - mga_set_start_address(crtc, (u32)gpu_addr); >> + mgag200_set_startadd(mdev, (unsigned long)gpu_addr); > drop this cast. > > >> >> return 0; >> >> @@ -894,6 +898,16 @@ static int mga_crtc_do_set_base(struct drm_crtc *crtc, >> static int mga_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, >> struct drm_framebuffer *old_fb) >> { >> + struct drm_device *dev = crtc->dev; >> + struct mga_device *mdev = dev->dev_private; >> + unsigned int count; >> + >> + while (RREG8(0x1fda) & 0x08) { } >> + while (!(RREG8(0x1fda) & 0x08)) { } >> + >> + count = RREG8(MGAREG_VCOUNT) + 2; >> + while (RREG8(MGAREG_VCOUNT) < count) { } >> + >> return mga_crtc_do_set_base(crtc, old_fb, x, y, 0); >> } > I do not really see why this code was lifter two functions up. > Before is was deep in mga_set_start_address(), now it is in > mga_crtc_mode_set_base(). > Puzzeled? Ah, that should have probably been explained in the commit message. The above code waits for the vsync flag to go up plus two scanlines. That way the pageflip happens during a vblank period. It's fairly inefficient AFAICT. I don't want this code in atomic modesetting, but didn't want to throw it away yet. So it's now in the non-atomic callback. Later when the atomic code calls mgag200_set_startadd(), it shouldn't busy-wait for vblanks. I still have a patch that adds vblank irq support to mgag200. I'd rather merge that instead of keeping this busy waiting in the driver. Best regards Thomas > > Sam > > > >> >> -- >> 2.26.0
Hi Am 30.04.20 um 10:23 schrieb Thomas Zimmermann: > Hi > > Am 29.04.20 um 20:20 schrieb Sam Ravnborg: >> Hi Thomas, >> >> On Wed, Apr 29, 2020 at 04:32:26PM +0200, Thomas Zimmermann wrote: >>> All register names and fields are now named according to the >>> MGA programming manuals. The function doesn't need the CRTC, so >>> callers pass in the device structure directly. The logging now >>> uses device-specific macros. >>> >>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>> --- >>> drivers/gpu/drm/mgag200/mgag200_drv.h | 5 ++ >>> drivers/gpu/drm/mgag200/mgag200_mode.c | 82 +++++++++++++++----------- >>> 2 files changed, 53 insertions(+), 34 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h >>> index 4403145e3593c..9b957d9fc7e04 100644 >>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h >>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h >>> @@ -61,6 +61,11 @@ >>> WREG8(MGAREG_CRTC_DATA, v); \ >>> } while (0) \ >>> >>> +#define RREG_ECRT(reg, v) \ >>> + do { \ >>> + WREG8(MGAREG_CRTCEXT_INDEX, reg); \ >>> + v = RREG8(MGAREG_CRTCEXT_DATA); \ >>> + } while (0) \ >>> >>> #define WREG_ECRT(reg, v) \ >>> do { \ >>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c >>> index 3d894b37a0812..b16a73c8617d6 100644 >>> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c >>> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c >>> @@ -819,49 +819,53 @@ static void mga_g200wb_commit(struct drm_crtc *crtc) >>> } >>> >>> /* >>> - This is how the framebuffer base address is stored in g200 cards: >>> - * Assume @offset is the gpu_addr variable of the framebuffer object >>> - * Then addr is the number of _pixels_ (not bytes) from the start of >>> - VRAM to the first pixel we want to display. (divided by 2 for 32bit >>> - framebuffers) >>> - * addr is stored in the CRTCEXT0, CRTCC and CRTCD registers >>> - addr<20> -> CRTCEXT0<6> >>> - addr<19-16> -> CRTCEXT0<3-0> >>> - addr<15-8> -> CRTCC<7-0> >>> - addr<7-0> -> CRTCD<7-0> >>> - CRTCEXT0 has to be programmed last to trigger an update and make the >>> - new addr variable take effect. >>> + * This is how the framebuffer base address is stored in g200 cards: >>> + * * Assume @offset is the gpu_addr variable of the framebuffer object >>> + * * Then addr is the number of _pixels_ (not bytes) from the start of >>> + * VRAM to the first pixel we want to display. (divided by 2 for 32bit >>> + * framebuffers) >>> + * * addr is stored in the CRTCEXT0, CRTCC and CRTCD registers >>> + * addr<20> -> CRTCEXT0<6> >>> + * addr<19-16> -> CRTCEXT0<3-0> >>> + * addr<15-8> -> CRTCC<7-0> >>> + * addr<7-0> -> CRTCD<7-0> >>> + * >>> + * CRTCEXT0 has to be programmed last to trigger an update and make the >>> + * new addr variable take effect. >>> */ >>> -static void mga_set_start_address(struct drm_crtc *crtc, unsigned offset) >>> +static void mgag200_set_startadd(struct mga_device *mdev, >>> + unsigned long offset) >>> { >>> - struct mga_device *mdev = crtc->dev->dev_private; >>> - u32 addr; >>> - int count; >>> - u8 crtcext0; >>> + struct drm_device *dev = mdev->dev; >>> + uint32_t startadd; >>> + uint8_t crtcc, crtcd, crtcext0; >>> >>> - while (RREG8(0x1fda) & 0x08); >>> - while (!(RREG8(0x1fda) & 0x08)); >>> + startadd = offset / 8; >>> >>> - count = RREG8(MGAREG_VCOUNT) + 2; >>> - while (RREG8(MGAREG_VCOUNT) < count); >>> - >>> - WREG8(MGAREG_CRTCEXT_INDEX, 0); >>> - crtcext0 = RREG8(MGAREG_CRTCEXT_DATA); >>> - crtcext0 &= 0xB0; >>> - addr = offset / 8; >>> - /* Can't store addresses any higher than that... >>> - but we also don't have more than 16MB of memory, so it should be fine. */ >>> - WARN_ON(addr > 0x1fffff); >>> - crtcext0 |= (!!(addr & (1<<20)))<<6; >>> - WREG_CRT(0x0d, (u8)(addr & 0xff)); >>> - WREG_CRT(0x0c, (u8)(addr >> 8) & 0xff); >>> - WREG_ECRT(0x0, ((u8)(addr >> 16) & 0xf) | crtcext0); >>> + /* >>> + * Can't store addresses any higher than that, but we also >>> + * don't have more than 16MB of memory, so it should be fine. >>> + */ >>> + drm_WARN_ON(dev, startadd > 0x1fffff); >>> + >>> + RREG_ECRT(0x00, crtcext0); >>> + >>> + crtcc = (startadd >> 8) & 0xff; >>> + crtcd = startadd & 0xff; >>> + crtcext0 &= 0xb0; >> >>> + crtcext0 |= ((startadd >> 14) & BIT(6)) | >> It is not so obvious that the value of bit 20 is stored in bit 6 here. >> >> Maybe: >> crtcext0 |= ((startadd & BIT(20) >> 14) | >> >> I would find the above easier to parse. > > Ok. I can change that. Reading the code again, I think it's better to keep it as it is. At least it's consistent with the rest of the function. Best regards Thomas > >> >>> + ((startadd >> 16) & 0x0f); >> >>> + >>> + WREG_CRT(0x0c, crtcc); >>> + WREG_CRT(0x0d, crtcd); >>> + WREG_ECRT(0x00, crtcext0); >>> } >>> >>> static int mga_crtc_do_set_base(struct drm_crtc *crtc, >>> struct drm_framebuffer *fb, >>> int x, int y, int atomic) >>> { >>> + struct mga_device *mdev = crtc->dev->dev_private; >> Could you use a crtc_to_mdev() macro here. >> So we avoid adding new users of dev_private? > > I introduce such a macro in a later patch. I guess I'll add a separate > patch for the macro and the conversion of all these dev_private references. > > >> >>> struct drm_gem_vram_object *gbo; >>> int ret; >>> s64 gpu_addr; >> Make this unsigned long and.. > > The function that returns gpu_addr can fail (but shouldn't) with a > negative error. That's why it's signed. > >> >>> @@ -882,7 +886,7 @@ static int mga_crtc_do_set_base(struct drm_crtc *crtc, >>> goto err_drm_gem_vram_unpin; >>> } >>> >>> - mga_set_start_address(crtc, (u32)gpu_addr); >>> + mgag200_set_startadd(mdev, (unsigned long)gpu_addr); >> drop this cast. >> >> >>> >>> return 0; >>> >>> @@ -894,6 +898,16 @@ static int mga_crtc_do_set_base(struct drm_crtc *crtc, >>> static int mga_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, >>> struct drm_framebuffer *old_fb) >>> { >>> + struct drm_device *dev = crtc->dev; >>> + struct mga_device *mdev = dev->dev_private; >>> + unsigned int count; >>> + >>> + while (RREG8(0x1fda) & 0x08) { } >>> + while (!(RREG8(0x1fda) & 0x08)) { } >>> + >>> + count = RREG8(MGAREG_VCOUNT) + 2; >>> + while (RREG8(MGAREG_VCOUNT) < count) { } >>> + >>> return mga_crtc_do_set_base(crtc, old_fb, x, y, 0); >>> } >> I do not really see why this code was lifter two functions up. >> Before is was deep in mga_set_start_address(), now it is in >> mga_crtc_mode_set_base(). >> Puzzeled? > > Ah, that should have probably been explained in the commit message. The > above code waits for the vsync flag to go up plus two scanlines. That > way the pageflip happens during a vblank period. > > It's fairly inefficient AFAICT. I don't want this code in atomic > modesetting, but didn't want to throw it away yet. So it's now in the > non-atomic callback. Later when the atomic code calls > mgag200_set_startadd(), it shouldn't busy-wait for vblanks. > > I still have a patch that adds vblank irq support to mgag200. I'd rather > merge that instead of keeping this busy waiting in the driver. > > Best regards > Thomas > > >> >> Sam >> >> >> >>> >>> -- >>> 2.26.0 >
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index 4403145e3593c..9b957d9fc7e04 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -61,6 +61,11 @@ WREG8(MGAREG_CRTC_DATA, v); \ } while (0) \ +#define RREG_ECRT(reg, v) \ + do { \ + WREG8(MGAREG_CRTCEXT_INDEX, reg); \ + v = RREG8(MGAREG_CRTCEXT_DATA); \ + } while (0) \ #define WREG_ECRT(reg, v) \ do { \ diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 3d894b37a0812..b16a73c8617d6 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -819,49 +819,53 @@ static void mga_g200wb_commit(struct drm_crtc *crtc) } /* - This is how the framebuffer base address is stored in g200 cards: - * Assume @offset is the gpu_addr variable of the framebuffer object - * Then addr is the number of _pixels_ (not bytes) from the start of - VRAM to the first pixel we want to display. (divided by 2 for 32bit - framebuffers) - * addr is stored in the CRTCEXT0, CRTCC and CRTCD registers - addr<20> -> CRTCEXT0<6> - addr<19-16> -> CRTCEXT0<3-0> - addr<15-8> -> CRTCC<7-0> - addr<7-0> -> CRTCD<7-0> - CRTCEXT0 has to be programmed last to trigger an update and make the - new addr variable take effect. + * This is how the framebuffer base address is stored in g200 cards: + * * Assume @offset is the gpu_addr variable of the framebuffer object + * * Then addr is the number of _pixels_ (not bytes) from the start of + * VRAM to the first pixel we want to display. (divided by 2 for 32bit + * framebuffers) + * * addr is stored in the CRTCEXT0, CRTCC and CRTCD registers + * addr<20> -> CRTCEXT0<6> + * addr<19-16> -> CRTCEXT0<3-0> + * addr<15-8> -> CRTCC<7-0> + * addr<7-0> -> CRTCD<7-0> + * + * CRTCEXT0 has to be programmed last to trigger an update and make the + * new addr variable take effect. */ -static void mga_set_start_address(struct drm_crtc *crtc, unsigned offset) +static void mgag200_set_startadd(struct mga_device *mdev, + unsigned long offset) { - struct mga_device *mdev = crtc->dev->dev_private; - u32 addr; - int count; - u8 crtcext0; + struct drm_device *dev = mdev->dev; + uint32_t startadd; + uint8_t crtcc, crtcd, crtcext0; - while (RREG8(0x1fda) & 0x08); - while (!(RREG8(0x1fda) & 0x08)); + startadd = offset / 8; - count = RREG8(MGAREG_VCOUNT) + 2; - while (RREG8(MGAREG_VCOUNT) < count); - - WREG8(MGAREG_CRTCEXT_INDEX, 0); - crtcext0 = RREG8(MGAREG_CRTCEXT_DATA); - crtcext0 &= 0xB0; - addr = offset / 8; - /* Can't store addresses any higher than that... - but we also don't have more than 16MB of memory, so it should be fine. */ - WARN_ON(addr > 0x1fffff); - crtcext0 |= (!!(addr & (1<<20)))<<6; - WREG_CRT(0x0d, (u8)(addr & 0xff)); - WREG_CRT(0x0c, (u8)(addr >> 8) & 0xff); - WREG_ECRT(0x0, ((u8)(addr >> 16) & 0xf) | crtcext0); + /* + * Can't store addresses any higher than that, but we also + * don't have more than 16MB of memory, so it should be fine. + */ + drm_WARN_ON(dev, startadd > 0x1fffff); + + RREG_ECRT(0x00, crtcext0); + + crtcc = (startadd >> 8) & 0xff; + crtcd = startadd & 0xff; + crtcext0 &= 0xb0; + crtcext0 |= ((startadd >> 14) & BIT(6)) | + ((startadd >> 16) & 0x0f); + + WREG_CRT(0x0c, crtcc); + WREG_CRT(0x0d, crtcd); + WREG_ECRT(0x00, crtcext0); } static int mga_crtc_do_set_base(struct drm_crtc *crtc, struct drm_framebuffer *fb, int x, int y, int atomic) { + struct mga_device *mdev = crtc->dev->dev_private; struct drm_gem_vram_object *gbo; int ret; s64 gpu_addr; @@ -882,7 +886,7 @@ static int mga_crtc_do_set_base(struct drm_crtc *crtc, goto err_drm_gem_vram_unpin; } - mga_set_start_address(crtc, (u32)gpu_addr); + mgag200_set_startadd(mdev, (unsigned long)gpu_addr); return 0; @@ -894,6 +898,16 @@ static int mga_crtc_do_set_base(struct drm_crtc *crtc, static int mga_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, struct drm_framebuffer *old_fb) { + struct drm_device *dev = crtc->dev; + struct mga_device *mdev = dev->dev_private; + unsigned int count; + + while (RREG8(0x1fda) & 0x08) { } + while (!(RREG8(0x1fda) & 0x08)) { } + + count = RREG8(MGAREG_VCOUNT) + 2; + while (RREG8(MGAREG_VCOUNT) < count) { } + return mga_crtc_do_set_base(crtc, old_fb, x, y, 0); }
All register names and fields are now named according to the MGA programming manuals. The function doesn't need the CRTC, so callers pass in the device structure directly. The logging now uses device-specific macros. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/mgag200/mgag200_drv.h | 5 ++ drivers/gpu/drm/mgag200/mgag200_mode.c | 82 +++++++++++++++----------- 2 files changed, 53 insertions(+), 34 deletions(-)