Re: [PATCH 15/17] drm/mgag200: Remove waiting from DPMS code

[Daniel Vetter <daniel@ffwll.ch>]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel