| Message ID | 20200429143238.10115-9-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:29PM +0200, Thomas Zimmermann wrote: > Set different fields in MISC in their rsp location in the code. This > patch also fixes a bug in the original code where the mode's SYNC flags > were never written into the MISC register. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/mgag200/mgag200_mode.c | 37 ++++++++++++++++++-------- > drivers/gpu/drm/mgag200/mgag200_reg.h | 5 +++- > 2 files changed, 30 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c > index 749ba6e420ac7..b5bb02e9f05d6 100644 > --- a/drivers/gpu/drm/mgag200/mgag200_mode.c > +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c > @@ -704,6 +704,8 @@ static int mga_g200er_set_plls(struct mga_device *mdev, long clock) > > static int mga_crtc_set_plls(struct mga_device *mdev, long clock) > { > + uint8_t misc; General comment. uint8_t and friends are for uapi stuff. kernel internal prefer u8 and friends. Would be good to clean this up in the drivire, maybe as a follow-up patch after is becomes atomic. > + > switch(mdev->type) { > case G200_SE_A: > case G200_SE_B: > @@ -724,6 +726,12 @@ static int mga_crtc_set_plls(struct mga_device *mdev, long clock) > return mga_g200er_set_plls(mdev, clock); > break; > } > + > + misc = RREG8(MGA_MISC_IN); > + misc &= ~GENMASK(3, 2); The code would be easier to read if we had a #define MGAREG_MISC_CLK_SEL_MASK GENMASK(3, 2) So the above became: misc &= ~MGAREG_MISC_CLK_SEL_MASK; Then it is more clear that the CLK_SEL bits are clared and then MGA_MSK is set. > + misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; > + WREG8(MGA_MISC_OUT, misc); > + > return 0; > } > > @@ -916,7 +924,7 @@ static void mgag200_set_mode_regs(struct mga_device *mdev, > { > unsigned int hdisplay, hsyncstart, hsyncend, htotal; > unsigned int vdisplay, vsyncstart, vsyncend, vtotal; > - uint8_t misc = 0; > + uint8_t misc; > uint8_t crtcext1, crtcext2, crtcext5; > > hdisplay = mode->hdisplay / 8 - 1; > @@ -933,10 +941,17 @@ static void mgag200_set_mode_regs(struct mga_device *mdev, > vsyncend = mode->vsync_end - 1; > vtotal = mode->vtotal - 2; > > + misc = RREG8(MGA_MISC_IN); > + > if (mode->flags & DRM_MODE_FLAG_NHSYNC) > - misc |= 0x40; > + misc |= MGAREG_MISC_HSYNCPOL; > + else > + misc &= ~MGAREG_MISC_HSYNCPOL; > + So the code just assumes DRM_MODE_FLAG_PHSYNC if DRM_MODE_FLAG_NHSYNC is not set. I think that is fine, but it also indicate how hoorible the flags definitions are in mode->flags > if (mode->flags & DRM_MODE_FLAG_NVSYNC) > - misc |= 0x80; > + misc |= MGAREG_MISC_VSYNCPOL; > + else > + misc &= ~MGAREG_MISC_VSYNCPOL; And this code was touched in previous patch, but I gess it is better to update it here. > > crtcext1 = (((htotal - 4) & 0x100) >> 8) | > ((hdisplay & 0x100) >> 7) | > @@ -982,6 +997,10 @@ static void mgag200_set_mode_regs(struct mga_device *mdev, > WREG_ECRT(0x01, crtcext1); > WREG_ECRT(0x02, crtcext2); > WREG_ECRT(0x05, crtcext5); > + > + WREG8(MGA_MISC_OUT, misc); > + > + mga_crtc_set_plls(mdev, mode->clock); > } > > static int mga_crtc_mode_set(struct drm_crtc *crtc, > @@ -1140,12 +1159,6 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc, > ext_vga[3] = ((1 << bppshift) - 1) | 0x80; > ext_vga[4] = 0; > > - /* Set pixel clocks */ > - misc = 0x2d; > - WREG8(MGA_MISC_OUT, misc); > - > - mga_crtc_set_plls(mdev, mode->clock); > - > WREG_ECRT(0, ext_vga[0]); > WREG_ECRT(3, ext_vga[3]); > WREG_ECRT(4, ext_vga[4]); > @@ -1161,9 +1174,11 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc, > } > > WREG_ECRT(0, ext_vga[0]); > - /* Enable mga pixel clock */ > - misc = 0x2d; > > + misc = RREG8(MGA_MISC_IN); > + misc |= MGAREG_MISC_IOADSEL | > + MGAREG_MISC_RAMMAPEN | > + MGAREG_MISC_HIGH_PG_SEL; > WREG8(MGA_MISC_OUT, misc); I am left puzzeled why there is several writes to MGA_MISC_OUT. The driver needs to read back and then write again. Would it be simpler to have one function that only took care of misc register update? > > mga_crtc_do_set_base(mdev, fb, old_fb); > diff --git a/drivers/gpu/drm/mgag200/mgag200_reg.h b/drivers/gpu/drm/mgag200/mgag200_reg.h > index c096a9d6bcbc1..89e12c55153cf 100644 > --- a/drivers/gpu/drm/mgag200/mgag200_reg.h > +++ b/drivers/gpu/drm/mgag200/mgag200_reg.h > @@ -16,10 +16,11 @@ > * MGA1064SG Mystique register file > */ > > - > #ifndef _MGA_REG_H_ > #define _MGA_REG_H_ > > +#include <linux/bits.h> > + > #define MGAREG_DWGCTL 0x1c00 > #define MGAREG_MACCESS 0x1c04 > /* the following is a mystique only register */ > @@ -227,6 +228,8 @@ > #define MGAREG_MISC_CLK_SEL_MGA_MSK (0x3 << 2) > #define MGAREG_MISC_VIDEO_DIS (0x1 << 4) > #define MGAREG_MISC_HIGH_PG_SEL (0x1 << 5) > +#define MGAREG_MISC_HSYNCPOL BIT(6) > +#define MGAREG_MISC_VSYNCPOL BIT(7) I like BIT(), but mixing (1 << N) and BIT() does not look nice. Oh, and do not get me started on GENMASK() :-) Sam > > /* MMIO VGA registers */ > #define MGAREG_SEQ_INDEX 0x1fc4 > -- > 2.26.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Am 03.05.20 um 17:34 schrieb Sam Ravnborg: > Hi Thomas. > > On Wed, Apr 29, 2020 at 04:32:29PM +0200, Thomas Zimmermann wrote: >> Set different fields in MISC in their rsp location in the code. This >> patch also fixes a bug in the original code where the mode's SYNC flags >> were never written into the MISC register. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/gpu/drm/mgag200/mgag200_mode.c | 37 ++++++++++++++++++-------- >> drivers/gpu/drm/mgag200/mgag200_reg.h | 5 +++- >> 2 files changed, 30 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c >> index 749ba6e420ac7..b5bb02e9f05d6 100644 >> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c >> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c >> @@ -704,6 +704,8 @@ static int mga_g200er_set_plls(struct mga_device *mdev, long clock) >> >> static int mga_crtc_set_plls(struct mga_device *mdev, long clock) >> { >> + uint8_t misc; > > General comment. > uint8_t and friends are for uapi stuff. > kernel internal prefer u8 and friends. Ok. > > Would be good to clean this up in the drivire, maybe as a follow-up > patch after is becomes atomic. > > >> + >> switch(mdev->type) { >> case G200_SE_A: >> case G200_SE_B: >> @@ -724,6 +726,12 @@ static int mga_crtc_set_plls(struct mga_device *mdev, long clock) >> return mga_g200er_set_plls(mdev, clock); >> break; >> } >> + >> + misc = RREG8(MGA_MISC_IN); >> + misc &= ~GENMASK(3, 2); > The code would be easier to read if we had a > #define MGAREG_MISC_CLK_SEL_MASK GENMASK(3, 2) > > So the above became: > misc &= ~MGAREG_MISC_CLK_SEL_MASK; > > Then it is more clear that the CLK_SEL bits are clared and then > MGA_MSK is set. Sure. > >> + misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; >> + WREG8(MGA_MISC_OUT, misc); >> + >> return 0; >> } >> >> @@ -916,7 +924,7 @@ static void mgag200_set_mode_regs(struct mga_device *mdev, >> { >> unsigned int hdisplay, hsyncstart, hsyncend, htotal; >> unsigned int vdisplay, vsyncstart, vsyncend, vtotal; >> - uint8_t misc = 0; >> + uint8_t misc; >> uint8_t crtcext1, crtcext2, crtcext5; >> >> hdisplay = mode->hdisplay / 8 - 1; >> @@ -933,10 +941,17 @@ static void mgag200_set_mode_regs(struct mga_device *mdev, >> vsyncend = mode->vsync_end - 1; >> vtotal = mode->vtotal - 2; >> >> + misc = RREG8(MGA_MISC_IN); >> + >> if (mode->flags & DRM_MODE_FLAG_NHSYNC) >> - misc |= 0x40; >> + misc |= MGAREG_MISC_HSYNCPOL; >> + else >> + misc &= ~MGAREG_MISC_HSYNCPOL; >> + > So the code just assumes DRM_MODE_FLAG_PHSYNC if > DRM_MODE_FLAG_NHSYNC is not set. > I think that is fine, but it also indicate how hoorible the > flags definitions are in mode->flags If polarity is not negative (i.e., bit set), it is positive (i.e., bit cleared). What else could you set in MISC? Having multiple flags in mode->flags that signal the same state is somewhat problematic. I expect that the consistency of a mode's flags is validated somewhere within the core. > > >> if (mode->flags & DRM_MODE_FLAG_NVSYNC) >> - misc |= 0x80; >> + misc |= MGAREG_MISC_VSYNCPOL; >> + else >> + misc &= ~MGAREG_MISC_VSYNCPOL; > And this code was touched in previous patch, but I gess it is better > to update it here. > >> >> crtcext1 = (((htotal - 4) & 0x100) >> 8) | >> ((hdisplay & 0x100) >> 7) | >> @@ -982,6 +997,10 @@ static void mgag200_set_mode_regs(struct mga_device *mdev, >> WREG_ECRT(0x01, crtcext1); >> WREG_ECRT(0x02, crtcext2); >> WREG_ECRT(0x05, crtcext5); >> + >> + WREG8(MGA_MISC_OUT, misc); >> + >> + mga_crtc_set_plls(mdev, mode->clock); >> } >> >> static int mga_crtc_mode_set(struct drm_crtc *crtc, >> @@ -1140,12 +1159,6 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc, >> ext_vga[3] = ((1 << bppshift) - 1) | 0x80; >> ext_vga[4] = 0; >> >> - /* Set pixel clocks */ >> - misc = 0x2d; >> - WREG8(MGA_MISC_OUT, misc); >> - >> - mga_crtc_set_plls(mdev, mode->clock); >> - >> WREG_ECRT(0, ext_vga[0]); >> WREG_ECRT(3, ext_vga[3]); >> WREG_ECRT(4, ext_vga[4]); >> @@ -1161,9 +1174,11 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc, >> } >> >> WREG_ECRT(0, ext_vga[0]); >> - /* Enable mga pixel clock */ >> - misc = 0x2d; >> >> + misc = RREG8(MGA_MISC_IN); >> + misc |= MGAREG_MISC_IOADSEL | >> + MGAREG_MISC_RAMMAPEN | >> + MGAREG_MISC_HIGH_PG_SEL; >> WREG8(MGA_MISC_OUT, misc); > > I am left puzzeled why there is several writes to MGA_MISC_OUT. > The driver needs to read back and then write again. > > Would it be simpler to have one function that only took care of > misc register update? I'm not quite sure what you mean. MISC contains all kinds of unrelated state. In the final atomic code, different state is set in different places. It's simple to have a function read-modify-write the content of MISC for the bits that it cares about. With multiple functions, that leads to some duplicated reads and write, but the code as a whole is simple. Best regards Thomas > >> >> mga_crtc_do_set_base(mdev, fb, old_fb); >> diff --git a/drivers/gpu/drm/mgag200/mgag200_reg.h b/drivers/gpu/drm/mgag200/mgag200_reg.h >> index c096a9d6bcbc1..89e12c55153cf 100644 >> --- a/drivers/gpu/drm/mgag200/mgag200_reg.h >> +++ b/drivers/gpu/drm/mgag200/mgag200_reg.h >> @@ -16,10 +16,11 @@ >> * MGA1064SG Mystique register file >> */ >> >> - >> #ifndef _MGA_REG_H_ >> #define _MGA_REG_H_ >> >> +#include <linux/bits.h> >> + >> #define MGAREG_DWGCTL 0x1c00 >> #define MGAREG_MACCESS 0x1c04 >> /* the following is a mystique only register */ >> @@ -227,6 +228,8 @@ >> #define MGAREG_MISC_CLK_SEL_MGA_MSK (0x3 << 2) >> #define MGAREG_MISC_VIDEO_DIS (0x1 << 4) >> #define MGAREG_MISC_HIGH_PG_SEL (0x1 << 5) >> +#define MGAREG_MISC_HSYNCPOL BIT(6) >> +#define MGAREG_MISC_VSYNCPOL BIT(7) > I like BIT(), but mixing (1 << N) and BIT() does not look nice. > Oh, and do not get me started on GENMASK() :-) > > Sam >> >> /* MMIO VGA registers */ >> #define MGAREG_SEQ_INDEX 0x1fc4 >> -- >> 2.26.0 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Thomas. > >> @@ -1161,9 +1174,11 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc, > >> } > >> > >> WREG_ECRT(0, ext_vga[0]); > >> - /* Enable mga pixel clock */ > >> - misc = 0x2d; > >> > >> + misc = RREG8(MGA_MISC_IN); > >> + misc |= MGAREG_MISC_IOADSEL | > >> + MGAREG_MISC_RAMMAPEN | > >> + MGAREG_MISC_HIGH_PG_SEL; > >> WREG8(MGA_MISC_OUT, misc); > > > > I am left puzzeled why there is several writes to MGA_MISC_OUT. > > The driver needs to read back and then write again. > > > > Would it be simpler to have one function that only took care of > > misc register update? > > I'm not quite sure what you mean. MISC contains all kinds of unrelated > state. In the final atomic code, different state is set in different > places. It's simple to have a function read-modify-write the content of > MISC for the bits that it cares about. With multiple functions, that > leads to some duplicated reads and write, but the code as a whole is simple. OK, when I looked at the code I initially got the impression that it was a bit random. But then I also switched between patch and code etc. The explanation above makes sense. Sam > > Best regards > Thomas > > > > >> > >> mga_crtc_do_set_base(mdev, fb, old_fb); > >> diff --git a/drivers/gpu/drm/mgag200/mgag200_reg.h b/drivers/gpu/drm/mgag200/mgag200_reg.h > >> index c096a9d6bcbc1..89e12c55153cf 100644 > >> --- a/drivers/gpu/drm/mgag200/mgag200_reg.h > >> +++ b/drivers/gpu/drm/mgag200/mgag200_reg.h > >> @@ -16,10 +16,11 @@ > >> * MGA1064SG Mystique register file > >> */ > >> > >> - > >> #ifndef _MGA_REG_H_ > >> #define _MGA_REG_H_ > >> > >> +#include <linux/bits.h> > >> + > >> #define MGAREG_DWGCTL 0x1c00 > >> #define MGAREG_MACCESS 0x1c04 > >> /* the following is a mystique only register */ > >> @@ -227,6 +228,8 @@ > >> #define MGAREG_MISC_CLK_SEL_MGA_MSK (0x3 << 2) > >> #define MGAREG_MISC_VIDEO_DIS (0x1 << 4) > >> #define MGAREG_MISC_HIGH_PG_SEL (0x1 << 5) > >> +#define MGAREG_MISC_HSYNCPOL BIT(6) > >> +#define MGAREG_MISC_VSYNCPOL BIT(7) > > I like BIT(), but mixing (1 << N) and BIT() does not look nice. > > Oh, and do not get me started on GENMASK() :-) > > > > Sam > >> > >> /* MMIO VGA registers */ > >> #define MGAREG_SEQ_INDEX 0x1fc4 > >> -- > >> 2.26.0 > >> > >> _______________________________________________ > >> dri-devel mailing list > >> dri-devel@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 >
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 749ba6e420ac7..b5bb02e9f05d6 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -704,6 +704,8 @@ static int mga_g200er_set_plls(struct mga_device *mdev, long clock) static int mga_crtc_set_plls(struct mga_device *mdev, long clock) { + uint8_t misc; + switch(mdev->type) { case G200_SE_A: case G200_SE_B: @@ -724,6 +726,12 @@ static int mga_crtc_set_plls(struct mga_device *mdev, long clock) return mga_g200er_set_plls(mdev, clock); break; } + + misc = RREG8(MGA_MISC_IN); + misc &= ~GENMASK(3, 2); + misc |= MGAREG_MISC_CLK_SEL_MGA_MSK; + WREG8(MGA_MISC_OUT, misc); + return 0; } @@ -916,7 +924,7 @@ static void mgag200_set_mode_regs(struct mga_device *mdev, { unsigned int hdisplay, hsyncstart, hsyncend, htotal; unsigned int vdisplay, vsyncstart, vsyncend, vtotal; - uint8_t misc = 0; + uint8_t misc; uint8_t crtcext1, crtcext2, crtcext5; hdisplay = mode->hdisplay / 8 - 1; @@ -933,10 +941,17 @@ static void mgag200_set_mode_regs(struct mga_device *mdev, vsyncend = mode->vsync_end - 1; vtotal = mode->vtotal - 2; + misc = RREG8(MGA_MISC_IN); + if (mode->flags & DRM_MODE_FLAG_NHSYNC) - misc |= 0x40; + misc |= MGAREG_MISC_HSYNCPOL; + else + misc &= ~MGAREG_MISC_HSYNCPOL; + if (mode->flags & DRM_MODE_FLAG_NVSYNC) - misc |= 0x80; + misc |= MGAREG_MISC_VSYNCPOL; + else + misc &= ~MGAREG_MISC_VSYNCPOL; crtcext1 = (((htotal - 4) & 0x100) >> 8) | ((hdisplay & 0x100) >> 7) | @@ -982,6 +997,10 @@ static void mgag200_set_mode_regs(struct mga_device *mdev, WREG_ECRT(0x01, crtcext1); WREG_ECRT(0x02, crtcext2); WREG_ECRT(0x05, crtcext5); + + WREG8(MGA_MISC_OUT, misc); + + mga_crtc_set_plls(mdev, mode->clock); } static int mga_crtc_mode_set(struct drm_crtc *crtc, @@ -1140,12 +1159,6 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc, ext_vga[3] = ((1 << bppshift) - 1) | 0x80; ext_vga[4] = 0; - /* Set pixel clocks */ - misc = 0x2d; - WREG8(MGA_MISC_OUT, misc); - - mga_crtc_set_plls(mdev, mode->clock); - WREG_ECRT(0, ext_vga[0]); WREG_ECRT(3, ext_vga[3]); WREG_ECRT(4, ext_vga[4]); @@ -1161,9 +1174,11 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc, } WREG_ECRT(0, ext_vga[0]); - /* Enable mga pixel clock */ - misc = 0x2d; + misc = RREG8(MGA_MISC_IN); + misc |= MGAREG_MISC_IOADSEL | + MGAREG_MISC_RAMMAPEN | + MGAREG_MISC_HIGH_PG_SEL; WREG8(MGA_MISC_OUT, misc); mga_crtc_do_set_base(mdev, fb, old_fb); diff --git a/drivers/gpu/drm/mgag200/mgag200_reg.h b/drivers/gpu/drm/mgag200/mgag200_reg.h index c096a9d6bcbc1..89e12c55153cf 100644 --- a/drivers/gpu/drm/mgag200/mgag200_reg.h +++ b/drivers/gpu/drm/mgag200/mgag200_reg.h @@ -16,10 +16,11 @@ * MGA1064SG Mystique register file */ - #ifndef _MGA_REG_H_ #define _MGA_REG_H_ +#include <linux/bits.h> + #define MGAREG_DWGCTL 0x1c00 #define MGAREG_MACCESS 0x1c04 /* the following is a mystique only register */ @@ -227,6 +228,8 @@ #define MGAREG_MISC_CLK_SEL_MGA_MSK (0x3 << 2) #define MGAREG_MISC_VIDEO_DIS (0x1 << 4) #define MGAREG_MISC_HIGH_PG_SEL (0x1 << 5) +#define MGAREG_MISC_HSYNCPOL BIT(6) +#define MGAREG_MISC_VSYNCPOL BIT(7) /* MMIO VGA registers */ #define MGAREG_SEQ_INDEX 0x1fc4
Set different fields in MISC in their rsp location in the code. This patch also fixes a bug in the original code where the mode's SYNC flags were never written into the MISC register. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/mgag200/mgag200_mode.c | 37 ++++++++++++++++++-------- drivers/gpu/drm/mgag200/mgag200_reg.h | 5 +++- 2 files changed, 30 insertions(+), 12 deletions(-)