Message ID | 20200429143238.10115-13-tzimmermann@suse.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm/mgag200: Convert to atomic modesetting | expand |
Hi Thomas. One nit about a bit name below. Acked-by: Sam Ravnborg <sam@ravnborg.org> On Wed, Apr 29, 2020 at 04:32:33PM +0200, Thomas Zimmermann wrote: > 5 > > The TAGFIFO state is now reset in mgag200_g200er_reset_tagfifo(). > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/mgag200/mgag200_drv.h | 6 ++++ > drivers/gpu/drm/mgag200/mgag200_mode.c | 45 +++++++++++++++++--------- > 2 files changed, 35 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h > index 9b957d9fc7e04..b10da90e0f35a 100644 > --- a/drivers/gpu/drm/mgag200/mgag200_drv.h > +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h > @@ -49,6 +49,12 @@ > WREG8(ATTR_DATA, v); \ > } while (0) \ > > +#define RREG_SEQ(reg, v) \ > + do { \ > + WREG8(MGAREG_SEQ_INDEX, reg); \ > + v = RREG8(MGAREG_SEQ_DATA); \ > + } while (0) \ > + > #define WREG_SEQ(reg, v) \ > do { \ > WREG8(MGAREG_SEQ_INDEX, reg); \ > diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c > index 73f7135cbb3d8..6b88c306ff4d7 100644 > --- a/drivers/gpu/drm/mgag200/mgag200_mode.c > +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c > @@ -1091,6 +1091,33 @@ static void mgag200_set_format_regs(struct mga_device *mdev, > WREG_ECRT(3, crtcext3); > } > > +static void mgag200_g200er_reset_tagfifo(struct mga_device *mdev) > +{ > + static uint32_t RESET_FLAG = 0x00200000; /* undocumented magic value */ > + u8 seq1; > + u32 memctl; > + > + /* screen off */ > + RREG_SEQ(0x01, seq1); > + seq1 |= 0x20; This looks like this: #define M_SEQ1_SCROFF 0x20 > + WREG_SEQ(0x01, seq1); > + > + memctl = RREG32(MGAREG_MEMCTL); > + > + memctl |= RESET_FLAG; > + WREG32(MGAREG_MEMCTL, memctl); > + > + udelay(1000); > + > + memctl &= ~RESET_FLAG; > + WREG32(MGAREG_MEMCTL, memctl); > + > + /* screen on */ > + RREG_SEQ(0x01, seq1); > + seq1 &= ~0x20; > + WREG_SEQ(0x01, seq1); Here seq1 is read again, the old code used the old value. I think new code is better. > +} > + > static int mga_crtc_mode_set(struct drm_crtc *crtc, > struct drm_display_mode *mode, > struct drm_display_mode *adjusted_mode, > @@ -1225,22 +1252,8 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc, > > mgag200_set_mode_regs(mdev, mode); > > - /* reset tagfifo */ > - if (mdev->type == G200_ER) { > - u32 mem_ctl = RREG32(MGAREG_MEMCTL); > - u8 seq1; > - > - /* screen off */ > - WREG8(MGAREG_SEQ_INDEX, 0x01); > - seq1 = RREG8(MGAREG_SEQ_DATA) | 0x20; > - WREG8(MGAREG_SEQ_DATA, seq1); > - > - WREG32(MGAREG_MEMCTL, mem_ctl | 0x00200000); > - udelay(1000); > - WREG32(MGAREG_MEMCTL, mem_ctl & ~0x00200000); > - > - WREG8(MGAREG_SEQ_DATA, seq1 & ~0x20); > - } > + if (mdev->type == G200_ER) > + mgag200_g200er_reset_tagfifo(mdev); > > > if (IS_G200_SE(mdev)) { > -- > 2.26.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-dev
Hi Am 03.05.20 um 18:25 schrieb Sam Ravnborg: > Hi Thomas. > > One nit about a bit name below. > Acked-by: Sam Ravnborg <sam@ravnborg.org> > > On Wed, Apr 29, 2020 at 04:32:33PM +0200, Thomas Zimmermann wrote: >> 5 >> >> The TAGFIFO state is now reset in mgag200_g200er_reset_tagfifo(). >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/gpu/drm/mgag200/mgag200_drv.h | 6 ++++ >> drivers/gpu/drm/mgag200/mgag200_mode.c | 45 +++++++++++++++++--------- >> 2 files changed, 35 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h >> index 9b957d9fc7e04..b10da90e0f35a 100644 >> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h >> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h >> @@ -49,6 +49,12 @@ >> WREG8(ATTR_DATA, v); \ >> } while (0) \ >> >> +#define RREG_SEQ(reg, v) \ >> + do { \ >> + WREG8(MGAREG_SEQ_INDEX, reg); \ >> + v = RREG8(MGAREG_SEQ_DATA); \ >> + } while (0) \ >> + >> #define WREG_SEQ(reg, v) \ >> do { \ >> WREG8(MGAREG_SEQ_INDEX, reg); \ >> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c >> index 73f7135cbb3d8..6b88c306ff4d7 100644 >> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c >> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c >> @@ -1091,6 +1091,33 @@ static void mgag200_set_format_regs(struct mga_device *mdev, >> WREG_ECRT(3, crtcext3); >> } >> >> +static void mgag200_g200er_reset_tagfifo(struct mga_device *mdev) >> +{ >> + static uint32_t RESET_FLAG = 0x00200000; /* undocumented magic value */ >> + u8 seq1; >> + u32 memctl; >> + >> + /* screen off */ >> + RREG_SEQ(0x01, seq1); >> + seq1 |= 0x20; > This looks like this: > #define M_SEQ1_SCROFF 0x20 > > >> + WREG_SEQ(0x01, seq1); >> + >> + memctl = RREG32(MGAREG_MEMCTL); >> + >> + memctl |= RESET_FLAG; >> + WREG32(MGAREG_MEMCTL, memctl); >> + >> + udelay(1000); >> + >> + memctl &= ~RESET_FLAG; >> + WREG32(MGAREG_MEMCTL, memctl); >> + >> + /* screen on */ >> + RREG_SEQ(0x01, seq1); >> + seq1 &= ~0x20; >> + WREG_SEQ(0x01, seq1); > Here seq1 is read again, the old code used the old value. > I think new code is better. You mean 'the old code was better,' right? Best regards Thomas > >> +} >> + >> static int mga_crtc_mode_set(struct drm_crtc *crtc, >> struct drm_display_mode *mode, >> struct drm_display_mode *adjusted_mode, >> @@ -1225,22 +1252,8 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc, >> >> mgag200_set_mode_regs(mdev, mode); >> >> - /* reset tagfifo */ >> - if (mdev->type == G200_ER) { >> - u32 mem_ctl = RREG32(MGAREG_MEMCTL); >> - u8 seq1; >> - >> - /* screen off */ >> - WREG8(MGAREG_SEQ_INDEX, 0x01); >> - seq1 = RREG8(MGAREG_SEQ_DATA) | 0x20; >> - WREG8(MGAREG_SEQ_DATA, seq1); >> - >> - WREG32(MGAREG_MEMCTL, mem_ctl | 0x00200000); >> - udelay(1000); >> - WREG32(MGAREG_MEMCTL, mem_ctl & ~0x00200000); >> - >> - WREG8(MGAREG_SEQ_DATA, seq1 & ~0x20); >> - } >> + if (mdev->type == G200_ER) >> + mgag200_g200er_reset_tagfifo(mdev); >> >> >> if (IS_G200_SE(mdev)) { >> -- >> 2.26.0 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-dev
Hi Thomas. > > > > > >> + WREG_SEQ(0x01, seq1); > >> + > >> + memctl = RREG32(MGAREG_MEMCTL); > >> + > >> + memctl |= RESET_FLAG; > >> + WREG32(MGAREG_MEMCTL, memctl); > >> + > >> + udelay(1000); > >> + > >> + memctl &= ~RESET_FLAG; > >> + WREG32(MGAREG_MEMCTL, memctl); > >> + > >> + /* screen on */ > >> + RREG_SEQ(0x01, seq1); > >> + seq1 &= ~0x20; > >> + WREG_SEQ(0x01, seq1); > > Here seq1 is read again, the old code used the old value. > > I think new code is better. > > You mean 'the old code was better,' right? Well, if there is no good reason to change it stick with the old code we know works. I was not sure what would happen with the register when reset was performed. So maybe reading back would be better, hence my comment. But re-using the old value gives full control of the register. So yeah, old code was better. Sam
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index 9b957d9fc7e04..b10da90e0f35a 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -49,6 +49,12 @@ WREG8(ATTR_DATA, v); \ } while (0) \ +#define RREG_SEQ(reg, v) \ + do { \ + WREG8(MGAREG_SEQ_INDEX, reg); \ + v = RREG8(MGAREG_SEQ_DATA); \ + } while (0) \ + #define WREG_SEQ(reg, v) \ do { \ WREG8(MGAREG_SEQ_INDEX, reg); \ diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 73f7135cbb3d8..6b88c306ff4d7 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -1091,6 +1091,33 @@ static void mgag200_set_format_regs(struct mga_device *mdev, WREG_ECRT(3, crtcext3); } +static void mgag200_g200er_reset_tagfifo(struct mga_device *mdev) +{ + static uint32_t RESET_FLAG = 0x00200000; /* undocumented magic value */ + u8 seq1; + u32 memctl; + + /* screen off */ + RREG_SEQ(0x01, seq1); + seq1 |= 0x20; + WREG_SEQ(0x01, seq1); + + memctl = RREG32(MGAREG_MEMCTL); + + memctl |= RESET_FLAG; + WREG32(MGAREG_MEMCTL, memctl); + + udelay(1000); + + memctl &= ~RESET_FLAG; + WREG32(MGAREG_MEMCTL, memctl); + + /* screen on */ + RREG_SEQ(0x01, seq1); + seq1 &= ~0x20; + WREG_SEQ(0x01, seq1); +} + static int mga_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode, @@ -1225,22 +1252,8 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc, mgag200_set_mode_regs(mdev, mode); - /* reset tagfifo */ - if (mdev->type == G200_ER) { - u32 mem_ctl = RREG32(MGAREG_MEMCTL); - u8 seq1; - - /* screen off */ - WREG8(MGAREG_SEQ_INDEX, 0x01); - seq1 = RREG8(MGAREG_SEQ_DATA) | 0x20; - WREG8(MGAREG_SEQ_DATA, seq1); - - WREG32(MGAREG_MEMCTL, mem_ctl | 0x00200000); - udelay(1000); - WREG32(MGAREG_MEMCTL, mem_ctl & ~0x00200000); - - WREG8(MGAREG_SEQ_DATA, seq1 & ~0x20); - } + if (mdev->type == G200_ER) + mgag200_g200er_reset_tagfifo(mdev); if (IS_G200_SE(mdev)) {
The TAGFIFO state is now reset in mgag200_g200er_reset_tagfifo(). Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/mgag200/mgag200_drv.h | 6 ++++ drivers/gpu/drm/mgag200/mgag200_mode.c | 45 +++++++++++++++++--------- 2 files changed, 35 insertions(+), 16 deletions(-)