| Message ID | 20200429143238.10115-16-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/mgag200: Convert to atomic modesetting | expand |
On Wed, Apr 29, 2020 at 04:32:36PM +0200, Thomas Zimmermann wrote: > The mgag200 drivers waits for the VSYNC flag to get signalled (i.e., > the page flip happens) before changing DPMS settings. This doesn't work > reliably if no mode has been programmed. Therefore remove the waiting > code. Synchronization with page flips should be done by DRM's vblank > handlers anyway. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> This looks a bit dangerous, hw might get angry if we drop these waits. Generally with atomic you should never have a situation in driver code where you expect the display to be on, but it isn't. So this should be fixable by making sure we're calling this dpms function at the right spot, e.g. for the enable path obviously the display is always going to be off, and for the disable path the display is guaranteed to be on. So maybe just a bool enable, or split the dpms function into two. The old legacy helpers where a lot more fast&loose in this regard. -Daniel > --- > drivers/gpu/drm/mgag200/mgag200_mode.c | 26 -------------------------- > 1 file changed, 26 deletions(-) > > diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c > index ee1cbe5decd71..884fc668a6dae 100644 > --- a/drivers/gpu/drm/mgag200/mgag200_mode.c > +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c > @@ -75,30 +75,6 @@ static void mga_crtc_load_lut(struct drm_crtc *crtc) > } > } > > -static inline void mga_wait_vsync(struct mga_device *mdev) > -{ > - unsigned long timeout = jiffies + HZ/10; > - unsigned int status = 0; > - > - do { > - status = RREG32(MGAREG_Status); > - } while ((status & 0x08) && time_before(jiffies, timeout)); > - timeout = jiffies + HZ/10; > - status = 0; > - do { > - status = RREG32(MGAREG_Status); > - } while (!(status & 0x08) && time_before(jiffies, timeout)); > -} > - > -static inline void mga_wait_busy(struct mga_device *mdev) > -{ > - unsigned long timeout = jiffies + HZ; > - unsigned int status = 0; > - do { > - status = RREG8(MGAREG_Status + 2); > - } while ((status & 0x01) && time_before(jiffies, timeout)); > -} > - > #define P_ARRAY_SIZE 9 > > static int mga_g200se_set_plls(struct mga_device *mdev, long clock) > @@ -1435,8 +1411,6 @@ static void mga_crtc_dpms(struct drm_crtc *crtc, int mode) > #endif > WREG8(MGAREG_SEQ_INDEX, 0x01); > seq1 |= RREG8(MGAREG_SEQ_DATA) & ~0x20; > - mga_wait_vsync(mdev); > - mga_wait_busy(mdev); > WREG8(MGAREG_SEQ_DATA, seq1); > msleep(20); > WREG8(MGAREG_CRTCEXT_INDEX, 0x01); > -- > 2.26.0 >
Hi Am 04.05.20 um 14:10 schrieb Daniel Vetter: > On Wed, Apr 29, 2020 at 04:32:36PM +0200, Thomas Zimmermann wrote: >> The mgag200 drivers waits for the VSYNC flag to get signalled (i.e., >> the page flip happens) before changing DPMS settings. This doesn't work >> reliably if no mode has been programmed. Therefore remove the waiting >> code. Synchronization with page flips should be done by DRM's vblank >> handlers anyway. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > > This looks a bit dangerous, hw might get angry if we drop these waits. > > Generally with atomic you should never have a situation in driver code > where you expect the display to be on, but it isn't. So this should be > fixable by making sure we're calling this dpms function at the right spot, > e.g. for the enable path obviously the display is always going to be off, > and for the disable path the display is guaranteed to be on. So maybe just > a bool enable, or split the dpms function into two. I think this code was taken from the X11 userspace driver, which does that same waiting. But it's waiting even for the DPMS_ON case. If the signal generation is disabled, why does it wait for the vsync flag? After a while the code times out from a timeout given in HZ/jiffies. I would have expected a value in usec. All this makes it somewhat dubious and I doubt that it's actually correct. If we want to keep the waiting, I agree on splitting the code into an enable and a disable function. The driver also busy waited during pageflips (see patch 5). That should really be done with interrupts. Best regards Thomas > > The old legacy helpers where a lot more fast&loose in this regard. > -Daniel > >> --- >> drivers/gpu/drm/mgag200/mgag200_mode.c | 26 -------------------------- >> 1 file changed, 26 deletions(-) >> >> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c >> index ee1cbe5decd71..884fc668a6dae 100644 >> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c >> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c >> @@ -75,30 +75,6 @@ static void mga_crtc_load_lut(struct drm_crtc *crtc) >> } >> } >> >> -static inline void mga_wait_vsync(struct mga_device *mdev) >> -{ >> - unsigned long timeout = jiffies + HZ/10; >> - unsigned int status = 0; >> - >> - do { >> - status = RREG32(MGAREG_Status); >> - } while ((status & 0x08) && time_before(jiffies, timeout)); >> - timeout = jiffies + HZ/10; >> - status = 0; >> - do { >> - status = RREG32(MGAREG_Status); >> - } while (!(status & 0x08) && time_before(jiffies, timeout)); >> -} >> - >> -static inline void mga_wait_busy(struct mga_device *mdev) >> -{ >> - unsigned long timeout = jiffies + HZ; >> - unsigned int status = 0; >> - do { >> - status = RREG8(MGAREG_Status + 2); >> - } while ((status & 0x01) && time_before(jiffies, timeout)); >> -} >> - >> #define P_ARRAY_SIZE 9 >> >> static int mga_g200se_set_plls(struct mga_device *mdev, long clock) >> @@ -1435,8 +1411,6 @@ static void mga_crtc_dpms(struct drm_crtc *crtc, int mode) >> #endif >> WREG8(MGAREG_SEQ_INDEX, 0x01); >> seq1 |= RREG8(MGAREG_SEQ_DATA) & ~0x20; >> - mga_wait_vsync(mdev); >> - mga_wait_busy(mdev); >> WREG8(MGAREG_SEQ_DATA, seq1); >> msleep(20); >> WREG8(MGAREG_CRTCEXT_INDEX, 0x01); >> -- >> 2.26.0 >> >
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index ee1cbe5decd71..884fc668a6dae 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -75,30 +75,6 @@ static void mga_crtc_load_lut(struct drm_crtc *crtc) } } -static inline void mga_wait_vsync(struct mga_device *mdev) -{ - unsigned long timeout = jiffies + HZ/10; - unsigned int status = 0; - - do { - status = RREG32(MGAREG_Status); - } while ((status & 0x08) && time_before(jiffies, timeout)); - timeout = jiffies + HZ/10; - status = 0; - do { - status = RREG32(MGAREG_Status); - } while (!(status & 0x08) && time_before(jiffies, timeout)); -} - -static inline void mga_wait_busy(struct mga_device *mdev) -{ - unsigned long timeout = jiffies + HZ; - unsigned int status = 0; - do { - status = RREG8(MGAREG_Status + 2); - } while ((status & 0x01) && time_before(jiffies, timeout)); -} - #define P_ARRAY_SIZE 9 static int mga_g200se_set_plls(struct mga_device *mdev, long clock) @@ -1435,8 +1411,6 @@ static void mga_crtc_dpms(struct drm_crtc *crtc, int mode) #endif WREG8(MGAREG_SEQ_INDEX, 0x01); seq1 |= RREG8(MGAREG_SEQ_DATA) & ~0x20; - mga_wait_vsync(mdev); - mga_wait_busy(mdev); WREG8(MGAREG_SEQ_DATA, seq1); msleep(20); WREG8(MGAREG_CRTCEXT_INDEX, 0x01);
The mgag200 drivers waits for the VSYNC flag to get signalled (i.e., the page flip happens) before changing DPMS settings. This doesn't work reliably if no mode has been programmed. Therefore remove the waiting code. Synchronization with page flips should be done by DRM's vblank handlers anyway. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/mgag200/mgag200_mode.c | 26 -------------------------- 1 file changed, 26 deletions(-)