[Intel-gfx] [PATCH 2/3] drm/radeon: Switch to drm_vblank_on/off

[Michel Dänzer]

On 27.05.2015 18:04, Daniel Vetter wrote:
> These should be functionally equivalent to the older per/post modeset
> functions, except that they block out drm_vblank_get right away.
> There's only the clock adjusting code (outside of pageflips) in
> readone which uses drm_vblank_get. But that code doesn't synchronize
> against concurrent modesets and instead handles any such races by
> waiting for the right vblank to arrive with a short timetout.
> 
> The longer-term plan here is to switch all kms drivers to
> drm_vblank_on/off so that common code like pending event cleanup can
> be done there, while drm_vblank_pre/post_modeset will be purely
> drm internal for the old UMS ioctl.
> 
> Note that the kerneldoc for pre/post_modeset is wrong since as Michel
> Dänzer correctly pointed out it works if only using pre/post_modeset.
> The trouble that lead to this comment is the very old version of
> drm_vblank_off to clear out pending events when disabling a pipe,
> which did seem to wreak havoc with the trick used by pre/post_modeset.
> Michel also expressed dissatisfaction with intel folks pushing new
> interfaces with bogus justifications. I still maintain that having a
> consistent set of vblank behaviour across kms drivers, separate from
> any old UMS functions is a useful goal.
> 
> Cc: Michel Dänzer <michel.daenzer at amd.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>

Can you describe at least one tangible benefit this change provides for
the radeon driver?

Because I'm afraid that this might cause subtle breakage, and since we
don't have any rigorous tests for this like in intel-gpu-tools (yet?),
it might be painful to track it down.

So, I'd like to have a good reason for taking the risk.


>  drivers/gpu/drm/radeon/atombios_crtc.c      | 4 ++--
>  drivers/gpu/drm/radeon/radeon_legacy_crtc.c | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c b/drivers/gpu/drm/radeon/atombios_crtc.c
> index 42b2ea3fdcf3..77912fd48b69 100644
> --- a/drivers/gpu/drm/radeon/atombios_crtc.c
> +++ b/drivers/gpu/drm/radeon/atombios_crtc.c
> @@ -274,13 +274,13 @@ void atombios_crtc_dpms(struct drm_crtc *crtc, int mode)
>  		if (ASIC_IS_DCE3(rdev) && !ASIC_IS_DCE6(rdev))
>  			atombios_enable_crtc_memreq(crtc, ATOM_ENABLE);
>  		atombios_blank_crtc(crtc, ATOM_DISABLE);
> -		drm_vblank_post_modeset(dev, radeon_crtc->crtc_id);
> +		drm_vblank_on(dev, radeon_crtc->crtc_id);
>  		radeon_crtc_load_lut(crtc);
>  		break;
>  	case DRM_MODE_DPMS_STANDBY:
>  	case DRM_MODE_DPMS_SUSPEND:
>  	case DRM_MODE_DPMS_OFF:
> -		drm_vblank_pre_modeset(dev, radeon_crtc->crtc_id);
> +		drm_vblank_off(dev, radeon_crtc->crtc_id);
>  		if (radeon_crtc->enabled)
>  			atombios_blank_crtc(crtc, ATOM_ENABLE);
>  		if (ASIC_IS_DCE3(rdev) && !ASIC_IS_DCE6(rdev))
> diff --git a/drivers/gpu/drm/radeon/radeon_legacy_crtc.c b/drivers/gpu/drm/radeon/radeon_legacy_crtc.c
> index 678b4386540d..4259e27f3983 100644
> --- a/drivers/gpu/drm/radeon/radeon_legacy_crtc.c
> +++ b/drivers/gpu/drm/radeon/radeon_legacy_crtc.c
> @@ -330,13 +330,13 @@ static void radeon_crtc_dpms(struct drm_crtc *crtc, int mode)
>  									 RADEON_CRTC_DISP_REQ_EN_B));
>  			WREG32_P(RADEON_CRTC_EXT_CNTL, crtc_ext_cntl, ~(mask | crtc_ext_cntl));
>  		}
> -		drm_vblank_post_modeset(dev, radeon_crtc->crtc_id);
> +		drm_vblank_on(dev, radeon_crtc->crtc_id);
>  		radeon_crtc_load_lut(crtc);
>  		break;
>  	case DRM_MODE_DPMS_STANDBY:
>  	case DRM_MODE_DPMS_SUSPEND:
>  	case DRM_MODE_DPMS_OFF:
> -		drm_vblank_pre_modeset(dev, radeon_crtc->crtc_id);
> +		drm_vblank_off(dev, radeon_crtc->crtc_id);
>  		if (radeon_crtc->crtc_id)
>  			WREG32_P(RADEON_CRTC2_GEN_CNTL, mask, ~(RADEON_CRTC2_EN | mask));
>  		else {
> 


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer