[3/4] drm/radeon: consolidate uvd/vce initialization, resume and suspend.

Commit Message

Jerome Glisse March 16, 2016, 12:48 p.m. UTC

From: Jérome Glisse <[e-mail]>

This consolidate uvd/vce into a common shape for all generation. It
also leverage the rdev->has_uvd flags to know what it is useless to
try to resume/suspend uvd/vce block.

There is no functional changes when there is no error. On error the
device driver will behave differently than before after this patch.
It should now safely ignore uvd/vce errors and keeps normal operation
of others engine. This is an improvement over current situation where
we have different behavior depending on GPU generation and on what
fails.

Finaly this is a preparatory step for a patch which allow to disable
uvd/vce as a driver option.

This have only been tested on southern island so please test it on
other generations (i do not have hardware handy for now).

Signed-off-by: Jérôme Glisse <[e-mail]>
Cc: Alex Deucher <[e-mail]>
Cc: Christian König <[e-mail]>
---
 drivers/gpu/drm/radeon/cik.c       | 226 ++++++++++++++++++++++------------
 drivers/gpu/drm/radeon/evergreen.c | 122 ++++++++++++++-----
 drivers/gpu/drm/radeon/ni.c        | 240 +++++++++++++++++++++++++------------
 drivers/gpu/drm/radeon/r600.c      | 113 ++++++++++++-----
 drivers/gpu/drm/radeon/rv770.c     | 122 ++++++++++++++-----
 drivers/gpu/drm/radeon/si.c        | 213 ++++++++++++++++++++------------
 drivers/gpu/drm/radeon/uvd_v4_2.c  |   5 +
 7 files changed, 724 insertions(+), 317 deletions(-)

Comments

Christian König March 16, 2016, 1:03 p.m. UTC | #1

Am 16.03.2016 um 13:48 schrieb Jérôme Glisse:
> From: Jérome Glisse <[e-mail]>
>
> This consolidate uvd/vce into a common shape for all generation. It
> also leverage the rdev->has_uvd flags to know what it is useless to
> try to resume/suspend uvd/vce block.
>
> There is no functional changes when there is no error. On error the
> device driver will behave differently than before after this patch.
> It should now safely ignore uvd/vce errors and keeps normal operation
> of others engine. This is an improvement over current situation where
> we have different behavior depending on GPU generation and on what
> fails.
>
> Finaly this is a preparatory step for a patch which allow to disable
> uvd/vce as a driver option.
>
> This have only been tested on southern island so please test it on
> other generations (i do not have hardware handy for now).
>
> Signed-off-by: Jérôme Glisse <[e-mail]>
> Cc: Alex Deucher <[e-mail]>
> Cc: Christian König <[e-mail]>

NAK, skipping UVD and VCE suspend/resume when initialization fails 
should already be implemented.

There might be some (quite some) bugs in there, but that doesn't justify 
reworking the initialization over all different generations. Especially 
since you don't have hardware to test all of them.

Just make sure that radeon_uvd/vce_fini() is called when something goes 
wrong and/or that the UVD/VCE BO is properly released.

Regards,
Christian.

> ---
>   drivers/gpu/drm/radeon/cik.c       | 226 ++++++++++++++++++++++------------
>   drivers/gpu/drm/radeon/evergreen.c | 122 ++++++++++++++-----
>   drivers/gpu/drm/radeon/ni.c        | 240 +++++++++++++++++++++++++------------
>   drivers/gpu/drm/radeon/r600.c      | 113 ++++++++++++-----
>   drivers/gpu/drm/radeon/rv770.c     | 122 ++++++++++++++-----
>   drivers/gpu/drm/radeon/si.c        | 213 ++++++++++++++++++++------------
>   drivers/gpu/drm/radeon/uvd_v4_2.c  |   5 +
>   7 files changed, 724 insertions(+), 317 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c
> index f2a4c0f..489e202 100644
> --- a/drivers/gpu/drm/radeon/cik.c
> +++ b/drivers/gpu/drm/radeon/cik.c
> @@ -8496,6 +8496,142 @@ restart_ih:
>   /*
>    * startup/shutdown callbacks
>    */
> +
> +static void cik_uvd_vce_init(struct radeon_device *rdev)
> +{
> +	struct radeon_ring *ring;
> +	int r;
> +
> +	if (!rdev->has_uvd)
> +		return;
> +
> +	r = radeon_uvd_init(rdev);
> +	if (r)
> +		goto error_uvd;
> +	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
> +	ring->ring_obj = NULL;
> +	r600_ring_init(rdev, ring, 4096);
> +
> +	r = radeon_vce_init(rdev);
> +	if (r)
> +		goto error_vce;
> +	ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
> +	ring->ring_obj = NULL;
> +	r600_ring_init(rdev, ring, 4096);
> +	ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
> +	ring->ring_obj = NULL;
> +	r600_ring_init(rdev, ring, 4096);
> +
> +	return;
> +
> +error_vce:
> +	radeon_uvd_fini(rdev);
> +error_uvd:
> +	dev_err(rdev->dev, "UVD/VCE init error (%d).\n", r);
> +	/* On error just disable everything. */
> +	rdev->has_uvd = 0;
> +}
> +
> +static void cik_uvd_vce_startup(struct radeon_device *rdev)
> +{
> +	int r;
> +
> +	if (!rdev->has_uvd)
> +		return;
> +
> +	r = uvd_v4_2_resume(rdev);
> +	if (r)
> +		goto error;
> +	r = radeon_fence_driver_start_ring(rdev, R600_RING_TYPE_UVD_INDEX);
> +	if (r)
> +		goto error_uvd;
> +
> +	r = radeon_vce_resume(rdev);
> +	if (r)
> +		goto error_uvd;
> +	r = vce_v2_0_resume(rdev);
> +	if (r)
> +		goto error_vce;
> +	r = radeon_fence_driver_start_ring(rdev, TN_RING_TYPE_VCE1_INDEX);
> +	if (r)
> +		goto error_vce;
> +	r = radeon_fence_driver_start_ring(rdev, TN_RING_TYPE_VCE2_INDEX);
> +	if (r)
> +		goto error_vce;
> +
> +	return;
> +
> +error_vce:
> +	radeon_vce_suspend(rdev);
> +error_uvd:
> +	radeon_uvd_suspend(rdev);
> +error:
> +	dev_err(rdev->dev, "UVD/VCE startup error (%d).\n", r);
> +	/* On error just disable everything. */
> +	radeon_vce_fini(rdev);
> +	radeon_uvd_fini(rdev);
> +	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
> +	rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
> +	rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;
> +	rdev->has_uvd = 0;
> +}
> +
> +static void cik_uvd_vce_resume(struct radeon_device *rdev)
> +{
> +	struct radeon_ring *ring;
> +	int r;
> +
> +	if (!rdev->has_uvd)
> +		return;
> +
> +	/* On uvd/vce error we disable uvd/vce so we should have bail above. */
> +	BUG_ON(!rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size);
> +	BUG_ON(!rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size);
> +	BUG_ON(!rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size);
> +
> +	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
> +	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, RADEON_CP_PACKET2);
> +	if (r)
> +		goto error;
> +	r = uvd_v1_0_init(rdev);
> +	if (r)
> +		goto error_uvd;
> +
> +	ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
> +	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, VCE_CMD_NO_OP);
> +	if (r)
> +		goto error_vce1;
> +	ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
> +	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, VCE_CMD_NO_OP);
> +	if (r)
> +		goto error_vce2;
> +	r = vce_v1_0_init(rdev);
> +	if (r)
> +		goto error_vce;
> +
> +	return;
> +
> +error_vce:
> +	radeon_ring_fini(rdev, &rdev->ring[TN_RING_TYPE_VCE2_INDEX]);
> +error_vce2:
> +	radeon_ring_fini(rdev, &rdev->ring[TN_RING_TYPE_VCE1_INDEX]);
> +error_vce1:
> +	uvd_v1_0_fini(rdev);
> +error_uvd:
> +	radeon_ring_fini(rdev, &rdev->ring[R600_RING_TYPE_UVD_INDEX]);
> +error:
> +	dev_err(rdev->dev, "UVD/VCE resume error (%d).\n", r);
> +	/* On error just disable everything. */
> +	radeon_uvd_suspend(rdev);
> +	radeon_vce_suspend(rdev);
> +	radeon_uvd_fini(rdev);
> +	radeon_vce_fini(rdev);
> +	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
> +	rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
> +	rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;
> +	rdev->has_uvd = 0;
> +}
> +
>   /**
>    * cik_startup - program the asic to a functional state
>    *
> @@ -8598,34 +8734,7 @@ static int cik_startup(struct radeon_device *rdev)
>   		return r;
>   	}
>   
> -	r = radeon_uvd_resume(rdev);
> -	if (!r) {
> -		r = uvd_v4_2_resume(rdev);
> -		if (!r) {
> -			r = radeon_fence_driver_start_ring(rdev,
> -							   R600_RING_TYPE_UVD_INDEX);
> -			if (r)
> -				dev_err(rdev->dev, "UVD fences init error (%d).\n", r);
> -		}
> -	}
> -	if (r)
> -		rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
> -
> -	r = radeon_vce_resume(rdev);
> -	if (!r) {
> -		r = vce_v2_0_resume(rdev);
> -		if (!r)
> -			r = radeon_fence_driver_start_ring(rdev,
> -							   TN_RING_TYPE_VCE1_INDEX);
> -		if (!r)
> -			r = radeon_fence_driver_start_ring(rdev,
> -							   TN_RING_TYPE_VCE2_INDEX);
> -	}
> -	if (r) {
> -		dev_err(rdev->dev, "VCE init error (%d).\n", r);
> -		rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
> -		rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;
> -	}
> +	cik_uvd_vce_startup(rdev);
>   
>   	/* Enable IRQ */
>   	if (!rdev->irq.installed) {
> @@ -8701,32 +8810,7 @@ static int cik_startup(struct radeon_device *rdev)
>   	if (r)
>   		return r;
>   
> -	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
> -	if (ring->ring_size) {
> -		r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
> -				     RADEON_CP_PACKET2);
> -		if (!r)
> -			r = uvd_v1_0_init(rdev);
> -		if (r)
> -			DRM_ERROR("radeon: failed initializing UVD (%d).\n", r);
> -	}
> -
> -	r = -ENOENT;
> -
> -	ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
> -	if (ring->ring_size)
> -		r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
> -				     VCE_CMD_NO_OP);
> -
> -	ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
> -	if (ring->ring_size)
> -		r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
> -				     VCE_CMD_NO_OP);
> -
> -	if (!r)
> -		r = vce_v1_0_init(rdev);
> -	else if (r != -ENOENT)
> -		DRM_ERROR("radeon: failed initializing VCE (%d).\n", r);
> +	cik_uvd_vce_resume(rdev);
>   
>   	r = radeon_ib_pool_init(rdev);
>   	if (r) {
> @@ -8802,9 +8886,11 @@ int cik_suspend(struct radeon_device *rdev)
>   	radeon_vm_manager_fini(rdev);
>   	cik_cp_enable(rdev, false);
>   	cik_sdma_enable(rdev, false);
> -	uvd_v1_0_fini(rdev);
> -	radeon_uvd_suspend(rdev);
> -	radeon_vce_suspend(rdev);
> +	if (rdev->has_uvd) {
> +		uvd_v1_0_fini(rdev);
> +		radeon_uvd_suspend(rdev);
> +		radeon_vce_suspend(rdev);
> +	}
>   	cik_fini_pg(rdev);
>   	cik_fini_cg(rdev);
>   	cik_irq_suspend(rdev);
> @@ -8930,23 +9016,7 @@ int cik_init(struct radeon_device *rdev)
>   	ring->ring_obj = NULL;
>   	r600_ring_init(rdev, ring, 256 * 1024);
>   
> -	r = radeon_uvd_init(rdev);
> -	if (!r) {
> -		ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
> -		ring->ring_obj = NULL;
> -		r600_ring_init(rdev, ring, 4096);
> -	}
> -
> -	r = radeon_vce_init(rdev);
> -	if (!r) {
> -		ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
> -		ring->ring_obj = NULL;
> -		r600_ring_init(rdev, ring, 4096);
> -
> -		ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
> -		ring->ring_obj = NULL;
> -		r600_ring_init(rdev, ring, 4096);
> -	}
> +	cik_uvd_vce_init(rdev);
>   
>   	rdev->ih.ring_obj = NULL;
>   	r600_ih_ring_init(rdev, 64 * 1024);
> @@ -9007,9 +9077,11 @@ void cik_fini(struct radeon_device *rdev)
>   	radeon_vm_manager_fini(rdev);
>   	radeon_ib_pool_fini(rdev);
>   	radeon_irq_kms_fini(rdev);
> -	uvd_v1_0_fini(rdev);
> -	radeon_uvd_fini(rdev);
> -	radeon_vce_fini(rdev);
> +	if (rdev->has_uvd) {
> +		uvd_v1_0_fini(rdev);
> +		radeon_uvd_fini(rdev);
> +		radeon_vce_fini(rdev);
> +	}
>   	cik_pcie_gart_fini(rdev);
>   	r600_vram_scratch_fini(rdev);
>   	radeon_gem_fini(rdev);
> diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
> index 76c4bdf..22854de 100644
> --- a/drivers/gpu/drm/radeon/evergreen.c
> +++ b/drivers/gpu/drm/radeon/evergreen.c
> @@ -5363,6 +5363,87 @@ restart_ih:
>   	return IRQ_HANDLED;
>   }
>   
> +static void evergreen_uvd_init(struct radeon_device *rdev)
> +{
> +	struct radeon_ring *ring;
> +	int r;
> +
> +	if (!rdev->has_uvd)
> +		return;
> +
> +	r = radeon_uvd_init(rdev);
> +	if (r)
> +		goto error_uvd;
> +	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
> +	ring->ring_obj = NULL;
> +	r600_ring_init(rdev, ring, 4096);
> +
> +	return;
> +
> +error_uvd:
> +	dev_err(rdev->dev, "UVD init error (%d).\n", r);
> +	/* On error just disable everything. */
> +	rdev->has_uvd = 0;
> +}
> +
> +static void evergreen_uvd_startup(struct radeon_device *rdev)
> +{
> +	int r;
> +
> +	if (!rdev->has_uvd)
> +		return;
> +
> +	r = uvd_v2_2_resume(rdev);
> +	if (r)
> +		goto error;
> +	r = radeon_fence_driver_start_ring(rdev, R600_RING_TYPE_UVD_INDEX);
> +	if (r)
> +		goto error_uvd;
> +
> +	return;
> +
> +error_uvd:
> +	radeon_uvd_suspend(rdev);
> +error:
> +	dev_err(rdev->dev, "UVD startup error (%d).\n", r);
> +	/* On error just disable everything. */
> +	radeon_uvd_fini(rdev);
> +	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
> +	rdev->has_uvd = 0;
> +}
> +
> +static void evergreen_uvd_resume(struct radeon_device *rdev)
> +{
> +	struct radeon_ring *ring;
> +	int r;
> +
> +	if (!rdev->has_uvd)
> +		return;
> +
> +	/* On uvd error we disable uvd so we should have bail above. */
> +	BUG_ON(!rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size);
> +
> +	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
> +	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, RADEON_CP_PACKET2);
> +	if (r)
> +		goto error;
> +	r = uvd_v1_0_init(rdev);
> +	if (r)
> +		goto error_uvd;
> +
> +	return;
> +
> +error_uvd:
> +	radeon_ring_fini(rdev, &rdev->ring[R600_RING_TYPE_UVD_INDEX]);
> +error:
> +	dev_err(rdev->dev, "UVD resume error (%d).\n", r);
> +	/* On error just disable everything. */
> +	radeon_uvd_suspend(rdev);
> +	radeon_uvd_fini(rdev);
> +	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
> +	rdev->has_uvd = 0;
> +}
> +
>   static int evergreen_startup(struct radeon_device *rdev)
>   {
>   	struct radeon_ring *ring;
> @@ -5427,16 +5508,7 @@ static int evergreen_startup(struct radeon_device *rdev)
>   		return r;
>   	}
>   
> -	r = uvd_v2_2_resume(rdev);
> -	if (!r) {
> -		r = radeon_fence_driver_start_ring(rdev,
> -						   R600_RING_TYPE_UVD_INDEX);
> -		if (r)
> -			dev_err(rdev->dev, "UVD fences init error (%d).\n", r);
> -	}
> -
> -	if (r)
> -		rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
> +	evergreen_uvd_startup(rdev);
>   
>   	/* Enable IRQ */
>   	if (!rdev->irq.installed) {
> @@ -5475,16 +5547,7 @@ static int evergreen_startup(struct radeon_device *rdev)
>   	if (r)
>   		return r;
>   
> -	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
> -	if (ring->ring_size) {
> -		r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
> -				     RADEON_CP_PACKET2);
> -		if (!r)
> -			r = uvd_v1_0_init(rdev);
> -
> -		if (r)
> -			DRM_ERROR("radeon: error initializing UVD (%d).\n", r);
> -	}
> +	evergreen_uvd_resume(rdev);
>   
>   	r = radeon_ib_pool_init(rdev);
>   	if (r) {
> @@ -5539,8 +5602,10 @@ int evergreen_suspend(struct radeon_device *rdev)
>   {
>   	radeon_pm_suspend(rdev);
>   	radeon_audio_fini(rdev);
> -	uvd_v1_0_fini(rdev);
> -	radeon_uvd_suspend(rdev);
> +	if (rdev->has_uvd) {
> +		uvd_v1_0_fini(rdev);
> +		radeon_uvd_suspend(rdev);
> +	}
>   	r700_cp_stop(rdev);
>   	r600_dma_stop(rdev);
>   	evergreen_irq_suspend(rdev);
> @@ -5641,12 +5706,7 @@ int evergreen_init(struct radeon_device *rdev)
>   	rdev->ring[R600_RING_TYPE_DMA_INDEX].ring_obj = NULL;
>   	r600_ring_init(rdev, &rdev->ring[R600_RING_TYPE_DMA_INDEX], 64 * 1024);
>   
> -	r = radeon_uvd_init(rdev);
> -	if (!r) {
> -		rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_obj = NULL;
> -		r600_ring_init(rdev, &rdev->ring[R600_RING_TYPE_UVD_INDEX],
> -			       4096);
> -	}
> +	evergreen_uvd_init(rdev);
>   
>   	rdev->ih.ring_obj = NULL;
>   	r600_ih_ring_init(rdev, 64 * 1024);
> @@ -5697,8 +5757,10 @@ void evergreen_fini(struct radeon_device *rdev)
>   	radeon_wb_fini(rdev);
>   	radeon_ib_pool_fini(rdev);
>   	radeon_irq_kms_fini(rdev);
> -	uvd_v1_0_fini(rdev);
> -	radeon_uvd_fini(rdev);
> +	if (rdev->has_uvd) {
> +		uvd_v1_0_fini(rdev);
> +		radeon_uvd_fini(rdev);
> +	}
>   	evergreen_pcie_gart_fini(rdev);
>   	r600_vram_scratch_fini(rdev);
>   	radeon_gem_fini(rdev);
> diff --git a/drivers/gpu/drm/radeon/ni.c b/drivers/gpu/drm/radeon/ni.c
> index b88d63c9..74f7316 100644
> --- a/drivers/gpu/drm/radeon/ni.c
> +++ b/drivers/gpu/drm/radeon/ni.c
> @@ -2002,6 +2002,154 @@ bool cayman_gfx_is_lockup(struct radeon_device *rdev, struct radeon_ring *ring)
>   	return radeon_ring_test_lockup(rdev, ring);
>   }
>   
> +static void cayman_uvd_vce_init(struct radeon_device *rdev)
> +{
> +	struct radeon_ring *ring;
> +	int r;
> +
> +	if (!rdev->has_uvd)
> +		return;
> +
> +	r = radeon_uvd_init(rdev);
> +	if (r)
> +		goto error_uvd;
> +	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
> +	ring->ring_obj = NULL;
> +	r600_ring_init(rdev, ring, 4096);
> +
> +	if (rdev->family != CHIP_ARUBA)
> +		return;
> +
> +	r = radeon_vce_init(rdev);
> +	if (r)
> +		goto error_vce;
> +	ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
> +	ring->ring_obj = NULL;
> +	r600_ring_init(rdev, ring, 4096);
> +	ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
> +	ring->ring_obj = NULL;
> +	r600_ring_init(rdev, ring, 4096);
> +
> +	return;
> +
> +error_vce:
> +	radeon_uvd_fini(rdev);
> +error_uvd:
> +	dev_err(rdev->dev, "UVD/VCE init error (%d).\n", r);
> +	/* On error just disable everything. */
> +	rdev->has_uvd = 0;
> +}
> +
> +static void cayman_uvd_vce_startup(struct radeon_device *rdev)
> +{
> +	int r;
> +
> +	if (!rdev->has_uvd)
> +		return;
> +
> +	r = uvd_v2_2_resume(rdev);
> +	if (r)
> +		goto error;
> +	r = radeon_fence_driver_start_ring(rdev, R600_RING_TYPE_UVD_INDEX);
> +	if (r)
> +		goto error_uvd;
> +
> +	/* VCE only CHIP_ARUBA */
> +	if (rdev->family != CHIP_ARUBA)
> +		return;
> +	r = radeon_vce_resume(rdev);
> +	if (r)
> +		goto error_uvd;
> +	r = vce_v1_0_resume(rdev);
> +	if (r)
> +		goto error_vce;
> +	r = radeon_fence_driver_start_ring(rdev, TN_RING_TYPE_VCE1_INDEX);
> +	if (r)
> +		goto error_vce;
> +	r = radeon_fence_driver_start_ring(rdev, TN_RING_TYPE_VCE2_INDEX);
> +	if (r)
> +		goto error_vce;
> +
> +	return;
> +
> +error_vce:
> +	radeon_vce_suspend(rdev);
> +error_uvd:
> +	radeon_uvd_suspend(rdev);
> +error:
> +	dev_err(rdev->dev, "UVD/VCE startup error (%d).\n", r);
> +	/* On error just disable everything. */
> +	radeon_uvd_fini(rdev);
> +	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
> +	if (rdev->family == CHIP_ARUBA) {
> +		radeon_vce_fini(rdev);
> +		rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
> +		rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;
> +	}
> +	rdev->has_uvd = 0;
> +}
> +
> +static void cayman_uvd_vce_resume(struct radeon_device *rdev)
> +{
> +	struct radeon_ring *ring;
> +	int r;
> +
> +	if (!rdev->has_uvd)
> +		return;
> +
> +	/* On uvd/vce error we disable uvd/vce so we should have bail above. */
> +	BUG_ON(!rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size);
> +
> +	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
> +	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, RADEON_CP_PACKET2);
> +	if (r)
> +		goto error;
> +	r = uvd_v1_0_init(rdev);
> +	if (r)
> +		goto error_uvd;
> +
> +	/* VCE only CHIP_ARUBA */
> +	if (rdev->family != CHIP_ARUBA)
> +		return;
> +	BUG_ON(!rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size);
> +	BUG_ON(!rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size);
> +	ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
> +	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, 0x0);
> +	if (r)
> +		goto error_vce1;
> +	ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
> +	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, 0x0);
> +	if (r)
> +		goto error_vce2;
> +	r = vce_v1_0_init(rdev);
> +	if (r)
> +		goto error_vce;
> +
> +	return;
> +
> +error_vce:
> +	radeon_ring_fini(rdev, &rdev->ring[TN_RING_TYPE_VCE2_INDEX]);
> +error_vce2:
> +	radeon_ring_fini(rdev, &rdev->ring[TN_RING_TYPE_VCE1_INDEX]);
> +error_vce1:
> +	uvd_v1_0_fini(rdev);
> +	radeon_vce_suspend(rdev);
> +error_uvd:
> +	radeon_ring_fini(rdev, &rdev->ring[R600_RING_TYPE_UVD_INDEX]);
> +error:
> +	dev_err(rdev->dev, "UVD/VCE resume error (%d).\n", r);
> +	/* On error just disable everything. */
> +	radeon_uvd_suspend(rdev);
> +	radeon_uvd_fini(rdev);
> +	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
> +	if (rdev->family == CHIP_ARUBA) {
> +		radeon_vce_fini(rdev);
> +		rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
> +		rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;
> +	}
> +	rdev->has_uvd = 0;
> +}
> +
>   static int cayman_startup(struct radeon_device *rdev)
>   {
>   	struct radeon_ring *ring = &rdev->ring[RADEON_RING_TYPE_GFX_INDEX];
> @@ -2056,34 +2204,7 @@ static int cayman_startup(struct radeon_device *rdev)
>   		return r;
>   	}
>   
> -	r = uvd_v2_2_resume(rdev);
> -	if (!r) {
> -		r = radeon_fence_driver_start_ring(rdev,
> -						   R600_RING_TYPE_UVD_INDEX);
> -		if (r)
> -			dev_err(rdev->dev, "UVD fences init error (%d).\n", r);
> -	}
> -	if (r)
> -		rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
> -
> -	if (rdev->family == CHIP_ARUBA) {
> -		r = radeon_vce_resume(rdev);
> -		if (!r)
> -			r = vce_v1_0_resume(rdev);
> -
> -		if (!r)
> -			r = radeon_fence_driver_start_ring(rdev,
> -							   TN_RING_TYPE_VCE1_INDEX);
> -		if (!r)
> -			r = radeon_fence_driver_start_ring(rdev,
> -							   TN_RING_TYPE_VCE2_INDEX);
> -
> -		if (r) {
> -			dev_err(rdev->dev, "VCE init error (%d).\n", r);
> -			rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
> -			rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;
> -		}
> -	}
> +	cayman_uvd_vce_startup(rdev);
>   
>   	r = radeon_fence_driver_start_ring(rdev, CAYMAN_RING_TYPE_CP1_INDEX);
>   	if (r) {
> @@ -2152,30 +2273,7 @@ static int cayman_startup(struct radeon_device *rdev)
>   	if (r)
>   		return r;
>   
> -	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
> -	if (ring->ring_size) {
> -		r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
> -				     RADEON_CP_PACKET2);
> -		if (!r)
> -			r = uvd_v1_0_init(rdev);
> -		if (r)
> -			DRM_ERROR("radeon: failed initializing UVD (%d).\n", r);
> -	}
> -
> -	if (rdev->family == CHIP_ARUBA) {
> -		ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
> -		if (ring->ring_size)
> -			r = radeon_ring_init(rdev, ring, ring->ring_size, 0, 0x0);
> -
> -		ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
> -		if (ring->ring_size)
> -			r = radeon_ring_init(rdev, ring, ring->ring_size, 0, 0x0);
> -
> -		if (!r)
> -			r = vce_v1_0_init(rdev);
> -		if (r)
> -			DRM_ERROR("radeon: failed initializing VCE (%d).\n", r);
> -	}
> +	cayman_uvd_vce_resume(rdev);
>   
>   	r = radeon_ib_pool_init(rdev);
>   	if (r) {
> @@ -2230,8 +2328,12 @@ int cayman_suspend(struct radeon_device *rdev)
>   	radeon_vm_manager_fini(rdev);
>   	cayman_cp_enable(rdev, false);
>   	cayman_dma_stop(rdev);
> -	uvd_v1_0_fini(rdev);
> -	radeon_uvd_suspend(rdev);
> +	if (rdev->has_uvd) {
> +		uvd_v1_0_fini(rdev);
> +		radeon_uvd_suspend(rdev);
> +		if (rdev->family == CHIP_ARUBA)
> +			radeon_vce_suspend(rdev);
> +	}
>   	evergreen_irq_suspend(rdev);
>   	radeon_wb_disable(rdev);
>   	cayman_pcie_gart_disable(rdev);
> @@ -2325,25 +2427,7 @@ int cayman_init(struct radeon_device *rdev)
>   	ring->ring_obj = NULL;
>   	r600_ring_init(rdev, ring, 64 * 1024);
>   
> -	r = radeon_uvd_init(rdev);
> -	if (!r) {
> -		ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
> -		ring->ring_obj = NULL;
> -		r600_ring_init(rdev, ring, 4096);
> -	}
> -
> -	if (rdev->family == CHIP_ARUBA) {
> -		r = radeon_vce_init(rdev);
> -		if (!r) {
> -			ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
> -			ring->ring_obj = NULL;
> -			r600_ring_init(rdev, ring, 4096);
> -
> -			ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
> -			ring->ring_obj = NULL;
> -			r600_ring_init(rdev, ring, 4096);
> -		}
> -	}
> +	cayman_uvd_vce_init(rdev);
>   
>   	rdev->ih.ring_obj = NULL;
>   	r600_ih_ring_init(rdev, 64 * 1024);
> @@ -2396,10 +2480,12 @@ void cayman_fini(struct radeon_device *rdev)
>   	radeon_vm_manager_fini(rdev);
>   	radeon_ib_pool_fini(rdev);
>   	radeon_irq_kms_fini(rdev);
> -	uvd_v1_0_fini(rdev);
> -	radeon_uvd_fini(rdev);
> -	if (rdev->family == CHIP_ARUBA)
> -		radeon_vce_fini(rdev);
> +	if (rdev->has_uvd) {
> +		uvd_v1_0_fini(rdev);
> +		radeon_uvd_fini(rdev);
> +		if (rdev->family == CHIP_ARUBA)
> +			radeon_vce_fini(rdev);
> +	}
>   	cayman_pcie_gart_fini(rdev);
>   	r600_vram_scratch_fini(rdev);
>   	radeon_gem_fini(rdev);
> diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
> index f86ab69..9d95bee 100644
> --- a/drivers/gpu/drm/radeon/r600.c
> +++ b/drivers/gpu/drm/radeon/r600.c
> @@ -3035,6 +3035,87 @@ void r600_clear_surface_reg(struct radeon_device *rdev, int reg)
>   	/* FIXME: implement */
>   }
>   
> +static void r600_uvd_init(struct radeon_device *rdev)
> +{
> +	struct radeon_ring *ring;
> +	int r;
> +
> +	if (!rdev->has_uvd)
> +		return;
> +
> +	r = radeon_uvd_init(rdev);
> +	if (r)
> +		goto error_uvd;
> +	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
> +	ring->ring_obj = NULL;
> +	r600_ring_init(rdev, ring, 4096);
> +
> +	return;
> +
> +error_uvd:
> +	dev_err(rdev->dev, "UVD init error (%d).\n", r);
> +	/* On error just disable everything. */
> +	rdev->has_uvd = 0;
> +}
> +
> +static void r600_uvd_startup(struct radeon_device *rdev)
> +{
> +	int r;
> +
> +	if (!rdev->has_uvd)
> +		return;
> +
> +	r = uvd_v1_0_resume(rdev);
> +	if (r)
> +		goto error;
> +	r = radeon_fence_driver_start_ring(rdev, R600_RING_TYPE_UVD_INDEX);
> +	if (r)
> +		goto error_uvd;
> +
> +	return;
> +
> +error_uvd:
> +	radeon_uvd_suspend(rdev);
> +error:
> +	dev_err(rdev->dev, "UVD startup error (%d).\n", r);
> +	/* On error just disable everything. */
> +	radeon_uvd_fini(rdev);
> +	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
> +	rdev->has_uvd = 0;
> +}
> +
> +static void r600_uvd_resume(struct radeon_device *rdev)
> +{
> +	struct radeon_ring *ring;
> +	int r;
> +
> +	if (!rdev->has_uvd)
> +		return;
> +
> +	/* On uvd error we disable uvd so we should have bail above. */
> +	BUG_ON(!rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size);
> +
> +	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
> +	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, RADEON_CP_PACKET2);
> +	if (r)
> +		goto error;
> +	r = uvd_v1_0_init(rdev);
> +	if (r)
> +		goto error_uvd;
> +
> +	return;
> +
> +error_uvd:
> +	radeon_ring_fini(rdev, &rdev->ring[R600_RING_TYPE_UVD_INDEX]);
> +error:
> +	dev_err(rdev->dev, "UVD resume error (%d).\n", r);
> +	/* On error just disable everything. */
> +	radeon_uvd_suspend(rdev);
> +	radeon_uvd_fini(rdev);
> +	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
> +	rdev->has_uvd = 0;
> +}
> +
>   static int r600_startup(struct radeon_device *rdev)
>   {
>   	struct radeon_ring *ring;
> @@ -3070,17 +3151,7 @@ static int r600_startup(struct radeon_device *rdev)
>   		return r;
>   	}
>   
> -	if (rdev->has_uvd) {
> -		r = uvd_v1_0_resume(rdev);
> -		if (!r) {
> -			r = radeon_fence_driver_start_ring(rdev, R600_RING_TYPE_UVD_INDEX);
> -			if (r) {
> -				dev_err(rdev->dev, "failed initializing UVD fences (%d).\n", r);
> -			}
> -		}
> -		if (r)
> -			rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
> -	}
> +	r600_uvd_startup(rdev);
>   
>   	/* Enable IRQ */
>   	if (!rdev->irq.installed) {
> @@ -3110,17 +3181,7 @@ static int r600_startup(struct radeon_device *rdev)
>   	if (r)
>   		return r;
>   
> -	if (rdev->has_uvd) {
> -		ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
> -		if (ring->ring_size) {
> -			r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
> -					     RADEON_CP_PACKET2);
> -			if (!r)
> -				r = uvd_v1_0_init(rdev);
> -			if (r)
> -				DRM_ERROR("radeon: failed initializing UVD (%d).\n", r);
> -		}
> -	}
> +	r600_uvd_resume(rdev);
>   
>   	r = radeon_ib_pool_init(rdev);
>   	if (r) {
> @@ -3264,13 +3325,7 @@ int r600_init(struct radeon_device *rdev)
>   	rdev->ring[RADEON_RING_TYPE_GFX_INDEX].ring_obj = NULL;
>   	r600_ring_init(rdev, &rdev->ring[RADEON_RING_TYPE_GFX_INDEX], 1024 * 1024);
>   
> -	if (rdev->has_uvd) {
> -		r = radeon_uvd_init(rdev);
> -		if (!r) {
> -			rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_obj = NULL;
> -			r600_ring_init(rdev, &rdev->ring[R600_RING_TYPE_UVD_INDEX], 4096);
> -		}
> -	}
> +	r600_uvd_init(rdev);
>   
>   	rdev->ih.ring_obj = NULL;
>   	r600_ih_ring_init(rdev, 64 * 1024);
> diff --git a/drivers/gpu/drm/radeon/rv770.c b/drivers/gpu/drm/radeon/rv770.c
> index 01ee96a..cf6e2ee 100644
> --- a/drivers/gpu/drm/radeon/rv770.c
> +++ b/drivers/gpu/drm/radeon/rv770.c
> @@ -1681,6 +1681,87 @@ static int rv770_mc_init(struct radeon_device *rdev)
>   	return 0;
>   }
>   
> +static void rv770_uvd_init(struct radeon_device *rdev)
> +{
> +	struct radeon_ring *ring;
> +	int r;
> +
> +	if (!rdev->has_uvd)
> +		return;
> +
> +	r = radeon_uvd_init(rdev);
> +	if (r)
> +		goto error_uvd;
> +	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
> +	ring->ring_obj = NULL;
> +	r600_ring_init(rdev, ring, 4096);
> +
> +	return;
> +
> +error_uvd:
> +	dev_err(rdev->dev, "UVD init error (%d).\n", r);
> +	/* On error just disable everything. */
> +	rdev->has_uvd = 0;
> +}
> +
> +static void rv770_uvd_startup(struct radeon_device *rdev)
> +{
> +	int r;
> +
> +	if (!rdev->has_uvd)
> +		return;
> +
> +	r = uvd_v2_2_resume(rdev);
> +	if (r)
> +		goto error;
> +	r = radeon_fence_driver_start_ring(rdev, R600_RING_TYPE_UVD_INDEX);
> +	if (r)
> +		goto error_uvd;
> +
> +	return;
> +
> +error_uvd:
> +	radeon_uvd_suspend(rdev);
> +error:
> +	dev_err(rdev->dev, "UVD startup error (%d).\n", r);
> +	/* On error just disable everything. */
> +	radeon_uvd_fini(rdev);
> +	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
> +	rdev->has_uvd = 0;
> +}
> +
> +static void rv770_uvd_resume(struct radeon_device *rdev)
> +{
> +	struct radeon_ring *ring;
> +	int r;
> +
> +	if (!rdev->has_uvd)
> +		return;
> +
> +	/* On uvd error we disable uvd so we should have bail above. */
> +	BUG_ON(!rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size);
> +
> +	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
> +	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, RADEON_CP_PACKET2);
> +	if (r)
> +		goto error;
> +	r = uvd_v1_0_init(rdev);
> +	if (r)
> +		goto error_uvd;
> +
> +	return;
> +
> +error_uvd:
> +	radeon_ring_fini(rdev, &rdev->ring[R600_RING_TYPE_UVD_INDEX]);
> +error:
> +	dev_err(rdev->dev, "UVD resume error (%d).\n", r);
> +	/* On error just disable everything. */
> +	radeon_uvd_suspend(rdev);
> +	radeon_uvd_fini(rdev);
> +	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
> +	rdev->has_uvd = 0;
> +}
> +
>   static int rv770_startup(struct radeon_device *rdev)
>   {
>   	struct radeon_ring *ring;
> @@ -1723,16 +1804,7 @@ static int rv770_startup(struct radeon_device *rdev)
>   		return r;
>   	}
>   
> -	r = uvd_v2_2_resume(rdev);
> -	if (!r) {
> -		r = radeon_fence_driver_start_ring(rdev,
> -						   R600_RING_TYPE_UVD_INDEX);
> -		if (r)
> -			dev_err(rdev->dev, "UVD fences init error (%d).\n", r);
> -	}
> -
> -	if (r)
> -		rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
> +	rv770_uvd_startup(rdev);
>   
>   	/* Enable IRQ */
>   	if (!rdev->irq.installed) {
> @@ -1772,16 +1844,7 @@ static int rv770_startup(struct radeon_device *rdev)
>   	if (r)
>   		return r;
>   
> -	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
> -	if (ring->ring_size) {
> -		r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
> -				     RADEON_CP_PACKET2);
> -		if (!r)
> -			r = uvd_v1_0_init(rdev);
> -
> -		if (r)
> -			DRM_ERROR("radeon: failed initializing UVD (%d).\n", r);
> -	}
> +	rv770_uvd_resume(rdev);
>   
>   	r = radeon_ib_pool_init(rdev);
>   	if (r) {
> @@ -1831,8 +1894,10 @@ int rv770_suspend(struct radeon_device *rdev)
>   {
>   	radeon_pm_suspend(rdev);
>   	radeon_audio_fini(rdev);
> -	uvd_v1_0_fini(rdev);
> -	radeon_uvd_suspend(rdev);
> +	if (rdev->has_uvd) {
> +		uvd_v1_0_fini(rdev);
> +		radeon_uvd_suspend(rdev);
> +	}
>   	r700_cp_stop(rdev);
>   	r600_dma_stop(rdev);
>   	r600_irq_suspend(rdev);
> @@ -1917,12 +1982,7 @@ int rv770_init(struct radeon_device *rdev)
>   	rdev->ring[R600_RING_TYPE_DMA_INDEX].ring_obj = NULL;
>   	r600_ring_init(rdev, &rdev->ring[R600_RING_TYPE_DMA_INDEX], 64 * 1024);
>   
> -	r = radeon_uvd_init(rdev);
> -	if (!r) {
> -		rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_obj = NULL;
> -		r600_ring_init(rdev, &rdev->ring[R600_RING_TYPE_UVD_INDEX],
> -			       4096);
> -	}
> +	rv770_uvd_init(rdev);
>   
>   	rdev->ih.ring_obj = NULL;
>   	r600_ih_ring_init(rdev, 64 * 1024);
> @@ -1957,8 +2017,10 @@ void rv770_fini(struct radeon_device *rdev)
>   	radeon_wb_fini(rdev);
>   	radeon_ib_pool_fini(rdev);
>   	radeon_irq_kms_fini(rdev);
> -	uvd_v1_0_fini(rdev);
> -	radeon_uvd_fini(rdev);
> +	if (rdev->has_uvd) {
> +		uvd_v1_0_fini(rdev);
> +		radeon_uvd_fini(rdev);
> +	}
>   	rv770_pcie_gart_fini(rdev);
>   	r600_vram_scratch_fini(rdev);
>   	radeon_gem_fini(rdev);
> diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c
> index e894be2..7d8d5f5 100644
> --- a/drivers/gpu/drm/radeon/si.c
> +++ b/drivers/gpu/drm/radeon/si.c
> @@ -6868,6 +6868,142 @@ restart_ih:
>   /*
>    * startup/shutdown callbacks
>    */
> +
> +static void si_uvd_vce_init(struct radeon_device *rdev)
> +{
> +	struct radeon_ring *ring;
> +	int r;
> +
> +	if (!rdev->has_uvd)
> +		return;
> +
> +	r = radeon_uvd_init(rdev);
> +	if (r)
> +		goto error_uvd;
> +	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
> +	ring->ring_obj = NULL;
> +	r600_ring_init(rdev, ring, 4096);
> +
> +	r = radeon_vce_init(rdev);
> +	if (r)
> +		goto error_vce;
> +	ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
> +	ring->ring_obj = NULL;
> +	r600_ring_init(rdev, ring, 4096);
> +	ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
> +	ring->ring_obj = NULL;
> +	r600_ring_init(rdev, ring, 4096);
> +
> +	return;
> +
> +error_vce:
> +	radeon_uvd_fini(rdev);
> +error_uvd:
> +	dev_err(rdev->dev, "UVD/VCE init error (%d).\n", r);
> +	/* On error just disable everything. */
> +	rdev->has_uvd = 0;
> +}
> +
> +static void si_uvd_vce_startup(struct radeon_device *rdev)
> +{
> +	int r;
> +
> +	if (!rdev->has_uvd)
> +		return;
> +
> +	r = uvd_v2_2_resume(rdev);
> +	if (r)
> +		goto error;
> +	r = radeon_fence_driver_start_ring(rdev, R600_RING_TYPE_UVD_INDEX);
> +	if (r)
> +		goto error_uvd;
> +
> +	r = radeon_vce_resume(rdev);
> +	if (r)
> +		goto error_uvd;
> +	r = vce_v1_0_resume(rdev);
> +	if (r)
> +		goto error_vce;
> +	r = radeon_fence_driver_start_ring(rdev, TN_RING_TYPE_VCE1_INDEX);
> +	if (r)
> +		goto error_vce;
> +	r = radeon_fence_driver_start_ring(rdev, TN_RING_TYPE_VCE2_INDEX);
> +	if (r)
> +		goto error_vce;
> +
> +	return;
> +
> +error_vce:
> +	radeon_vce_suspend(rdev);
> +error_uvd:
> +	radeon_uvd_suspend(rdev);
> +error:
> +	dev_err(rdev->dev, "UVD/VCE startup error (%d).\n", r);
> +	/* On error just disable everything. */
> +	radeon_vce_fini(rdev);
> +	radeon_uvd_fini(rdev);
> +	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
> +	rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
> +	rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;
> +	rdev->has_uvd = 0;
> +}
> +
> +static void si_uvd_vce_resume(struct radeon_device *rdev)
> +{
> +	struct radeon_ring *ring;
> +	int r;
> +
> +	if (!rdev->has_uvd)
> +		return;
> +
> +	/* On uvd/vce error we disable uvd/vce so we should have bail above. */
> +	BUG_ON(!rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size);
> +	BUG_ON(!rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size);
> +	BUG_ON(!rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size);
> +
> +	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
> +	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, RADEON_CP_PACKET2);
> +	if (r)
> +		goto error;
> +	r = uvd_v1_0_init(rdev);
> +	if (r)
> +		goto error_uvd;
> +
> +	ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
> +	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, VCE_CMD_NO_OP);
> +	if (r)
> +		goto error_vce1;
> +	ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
> +	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, VCE_CMD_NO_OP);
> +	if (r)
> +		goto error_vce2;
> +	r = vce_v1_0_init(rdev);
> +	if (r)
> +		goto error_vce;
> +
> +	return;
> +
> +error_vce:
> +	radeon_ring_fini(rdev, &rdev->ring[TN_RING_TYPE_VCE2_INDEX]);
> +error_vce2:
> +	radeon_ring_fini(rdev, &rdev->ring[TN_RING_TYPE_VCE1_INDEX]);
> +error_vce1:
> +	uvd_v1_0_fini(rdev);
> +error_uvd:
> +	radeon_ring_fini(rdev, &rdev->ring[R600_RING_TYPE_UVD_INDEX]);
> +error:
> +	dev_err(rdev->dev, "UVD/VCE resume error (%d).\n", r);
> +	/* On error just disable everything. */
> +	radeon_uvd_suspend(rdev);
> +	radeon_vce_suspend(rdev);
> +	radeon_uvd_fini(rdev);
> +	radeon_vce_fini(rdev);
> +	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
> +	rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
> +	rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;
> +	rdev->has_uvd = 0;
> +}
> +
>   static int si_startup(struct radeon_device *rdev)
>   {
>   	struct radeon_ring *ring;
> @@ -6946,33 +7082,7 @@ static int si_startup(struct radeon_device *rdev)
>   		return r;
>   	}
>   
> -	if (rdev->has_uvd) {
> -		r = uvd_v2_2_resume(rdev);
> -		if (!r) {
> -			r = radeon_fence_driver_start_ring(rdev,
> -							   R600_RING_TYPE_UVD_INDEX);
> -			if (r)
> -				dev_err(rdev->dev, "UVD fences init error (%d).\n", r);
> -		}
> -		if (r)
> -			rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
> -	}
> -
> -	r = radeon_vce_resume(rdev);
> -	if (!r) {
> -		r = vce_v1_0_resume(rdev);
> -		if (!r)
> -			r = radeon_fence_driver_start_ring(rdev,
> -							   TN_RING_TYPE_VCE1_INDEX);
> -		if (!r)
> -			r = radeon_fence_driver_start_ring(rdev,
> -							   TN_RING_TYPE_VCE2_INDEX);
> -	}
> -	if (r) {
> -		dev_err(rdev->dev, "VCE init error (%d).\n", r);
> -		rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
> -		rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;
> -	}
> +	si_uvd_vce_startup(rdev);
>   
>   	/* Enable IRQ */
>   	if (!rdev->irq.installed) {
> @@ -7030,34 +7140,7 @@ static int si_startup(struct radeon_device *rdev)
>   	if (r)
>   		return r;
>   
> -	if (rdev->has_uvd) {
> -		ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
> -		if (ring->ring_size) {
> -			r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
> -					     RADEON_CP_PACKET2);
> -			if (!r)
> -				r = uvd_v1_0_init(rdev);
> -			if (r)
> -				DRM_ERROR("radeon: failed initializing UVD (%d).\n", r);
> -		}
> -	}
> -
> -	r = -ENOENT;
> -
> -	ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
> -	if (ring->ring_size)
> -		r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
> -				     VCE_CMD_NO_OP);
> -
> -	ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
> -	if (ring->ring_size)
> -		r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
> -				     VCE_CMD_NO_OP);
> -
> -	if (!r)
> -		r = vce_v1_0_init(rdev);
> -	else if (r != -ENOENT)
> -		DRM_ERROR("radeon: failed initializing VCE (%d).\n", r);
> +	si_uvd_vce_resume(rdev);
>   
>   	r = radeon_ib_pool_init(rdev);
>   	if (r) {
> @@ -7216,25 +7299,7 @@ int si_init(struct radeon_device *rdev)
>   	ring->ring_obj = NULL;
>   	r600_ring_init(rdev, ring, 64 * 1024);
>   
> -	if (rdev->has_uvd) {
> -		r = radeon_uvd_init(rdev);
> -		if (!r) {
> -			ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
> -			ring->ring_obj = NULL;
> -			r600_ring_init(rdev, ring, 4096);
> -		}
> -	}
> -
> -	r = radeon_vce_init(rdev);
> -	if (!r) {
> -		ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
> -		ring->ring_obj = NULL;
> -		r600_ring_init(rdev, ring, 4096);
> -
> -		ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
> -		ring->ring_obj = NULL;
> -		r600_ring_init(rdev, ring, 4096);
> -	}
> +	si_uvd_vce_init(rdev);
>   
>   	rdev->ih.ring_obj = NULL;
>   	r600_ih_ring_init(rdev, 64 * 1024);
> diff --git a/drivers/gpu/drm/radeon/uvd_v4_2.c b/drivers/gpu/drm/radeon/uvd_v4_2.c
> index d04d507..d7e4807 100644
> --- a/drivers/gpu/drm/radeon/uvd_v4_2.c
> +++ b/drivers/gpu/drm/radeon/uvd_v4_2.c
> @@ -39,6 +39,11 @@ int uvd_v4_2_resume(struct radeon_device *rdev)
>   {
>   	uint64_t addr;
>   	uint32_t size;
> +	int r;
> +
> +	r = radeon_uvd_resume(rdev);
> +	if (r)
> +		return r;
>   
>   	/* programm the VCPU memory controller bits 0-27 */
>   	addr = rdev->uvd.gpu_addr >> 3;

Jerome Glisse March 16, 2016, 2:59 p.m. UTC | #2

On Wed, Mar 16, 2016 at 2:03 PM, Christian König
<[e-mail]> wrote:
> Am 16.03.2016 um 13:48 schrieb Jérôme Glisse:
>>
>> From: Jérome Glisse <[e-mail]>
>>
>> This consolidate uvd/vce into a common shape for all generation. It
>> also leverage the rdev->has_uvd flags to know what it is useless to
>> try to resume/suspend uvd/vce block.
>>
>> There is no functional changes when there is no error. On error the
>> device driver will behave differently than before after this patch.
>> It should now safely ignore uvd/vce errors and keeps normal operation
>> of others engine. This is an improvement over current situation where
>> we have different behavior depending on GPU generation and on what
>> fails.
>>
>> Finaly this is a preparatory step for a patch which allow to disable
>> uvd/vce as a driver option.
>>
>> This have only been tested on southern island so please test it on
>> other generations (i do not have hardware handy for now).
>>
>> Signed-off-by: Jérôme Glisse <[e-mail]>
>> Cc: Alex Deucher <[e-mail]>
>> Cc: Christian König <[e-mail]>
>
>
> NAK, skipping UVD and VCE suspend/resume when initialization fails should
> already be implemented.
>
> There might be some (quite some) bugs in there, but that doesn't justify
> reworking the initialization over all different generations. Especially
> since you don't have hardware to test all of them.
>
> Just make sure that radeon_uvd/vce_fini() is called when something goes
> wrong and/or that the UVD/VCE BO is properly released.
>
> Regards,
> Christian.

Current code is a mess when it comes to handling error related to
uvd/vce. This patch consolidate control flow in something easy to
follow. You can check that there is absolulety no control flow change
for the case where uvd/vce works and thus it does not break anything
that works. It will only gracefully fails and cleanup if things go
wrong. So while i have not tested on other hw i am confident that this
does not introduce regression.

I tried to do it without consolidation but it ended up in adding even
more if() levels that line did begins after 80colums. So please
reconsider because this is an improvement over existing code.

Jérôme

Christian König March 16, 2016, 3:19 p.m. UTC | #3

Am 16.03.2016 um 15:59 schrieb Jerome Glisse:
> On Wed, Mar 16, 2016 at 2:03 PM, Christian König
> <[e-mail]> wrote:
>> Am 16.03.2016 um 13:48 schrieb Jérôme Glisse:
>>> From: Jérome Glisse <[e-mail]>
>>>
>>> This consolidate uvd/vce into a common shape for all generation. It
>>> also leverage the rdev->has_uvd flags to know what it is useless to
>>> try to resume/suspend uvd/vce block.
>>>
>>> There is no functional changes when there is no error. On error the
>>> device driver will behave differently than before after this patch.
>>> It should now safely ignore uvd/vce errors and keeps normal operation
>>> of others engine. This is an improvement over current situation where
>>> we have different behavior depending on GPU generation and on what
>>> fails.
>>>
>>> Finaly this is a preparatory step for a patch which allow to disable
>>> uvd/vce as a driver option.
>>>
>>> This have only been tested on southern island so please test it on
>>> other generations (i do not have hardware handy for now).
>>>
>>> Signed-off-by: Jérôme Glisse <[e-mail]>
>>> Cc: Alex Deucher <[e-mail]>
>>> Cc: Christian König <[e-mail]>
>>
>> NAK, skipping UVD and VCE suspend/resume when initialization fails should
>> already be implemented.
>>
>> There might be some (quite some) bugs in there, but that doesn't justify
>> reworking the initialization over all different generations. Especially
>> since you don't have hardware to test all of them.
>>
>> Just make sure that radeon_uvd/vce_fini() is called when something goes
>> wrong and/or that the UVD/VCE BO is properly released.
>>
>> Regards,
>> Christian.
> Current code is a mess when it comes to handling error related to
> uvd/vce. This patch consolidate control flow in something easy to
> follow. You can check that there is absolulety no control flow change
> for the case where uvd/vce works and thus it does not break anything
> that works. It will only gracefully fails and cleanup if things go
> wrong. So while i have not tested on other hw i am confident that this
> does not introduce regression.
>
> I tried to do it without consolidation but it ended up in adding even
> more if() levels that line did begins after 80colums. So please
> reconsider because this is an improvement over existing code.

Well then please point out at the example of the SI or CIK code what 
exactly is missing here.

Please also note that VCE/UVD has dependencies on power management, so 
that when they are once initialized they should NOT be turned off again.

I only briefly skimmed over your patch, but it actually looks like to me 
that you broken that by trying to cleanup the initialization routine.

Regards,
Christian.

>
> Jérôme

Jerome Glisse March 16, 2016, 3:56 p.m. UTC | #4

On Wed, Mar 16, 2016 at 04:19:16PM +0100, Christian König wrote:
> Am 16.03.2016 um 15:59 schrieb Jerome Glisse:
> >On Wed, Mar 16, 2016 at 2:03 PM, Christian König
> ><[e-mail]> wrote:
> >>Am 16.03.2016 um 13:48 schrieb Jérôme Glisse:
> >>>From: Jérome Glisse <[e-mail]>
> >>>
> >>>This consolidate uvd/vce into a common shape for all generation. It
> >>>also leverage the rdev->has_uvd flags to know what it is useless to
> >>>try to resume/suspend uvd/vce block.
> >>>
> >>>There is no functional changes when there is no error. On error the
> >>>device driver will behave differently than before after this patch.
> >>>It should now safely ignore uvd/vce errors and keeps normal operation
> >>>of others engine. This is an improvement over current situation where
> >>>we have different behavior depending on GPU generation and on what
> >>>fails.
> >>>
> >>>Finaly this is a preparatory step for a patch which allow to disable
> >>>uvd/vce as a driver option.
> >>>
> >>>This have only been tested on southern island so please test it on
> >>>other generations (i do not have hardware handy for now).
> >>>
> >>>Signed-off-by: Jérôme Glisse <[e-mail]>
> >>>Cc: Alex Deucher <[e-mail]>
> >>>Cc: Christian König <[e-mail]>
> >>
> >>NAK, skipping UVD and VCE suspend/resume when initialization fails should
> >>already be implemented.
> >>
> >>There might be some (quite some) bugs in there, but that doesn't justify
> >>reworking the initialization over all different generations. Especially
> >>since you don't have hardware to test all of them.
> >>
> >>Just make sure that radeon_uvd/vce_fini() is called when something goes
> >>wrong and/or that the UVD/VCE BO is properly released.
> >>
> >>Regards,
> >>Christian.
> >Current code is a mess when it comes to handling error related to
> >uvd/vce. This patch consolidate control flow in something easy to
> >follow. You can check that there is absolulety no control flow change
> >for the case where uvd/vce works and thus it does not break anything
> >that works. It will only gracefully fails and cleanup if things go
> >wrong. So while i have not tested on other hw i am confident that this
> >does not introduce regression.
> >
> >I tried to do it without consolidation but it ended up in adding even
> >more if() levels that line did begins after 80colums. So please
> >reconsider because this is an improvement over existing code.
> 
> Well then please point out at the example of the SI or CIK code what exactly
> is missing here.

Going from :
	if (rdev->has_uvd) {
		r = uvd_v2_2_resume(rdev);
		if (!r) {
			r = radeon_fence_driver_start_ring(rdev,
							   R600_RING_TYPE_UVD_INDEX);
			if (r)
				dev_err(rdev->dev, "UVD fences init error (%d).\n", r);
		}
		if (r)
			rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
	}
	r = radeon_vce_resume(rdev);
	if (!r) {
		r = vce_v1_0_resume(rdev);
		if (!r)
			r = radeon_fence_driver_start_ring(rdev,
							   TN_RING_TYPE_VCE1_INDEX);
		if (!r)
			r = radeon_fence_driver_start_ring(rdev,
							   TN_RING_TYPE_VCE2_INDEX);
	}
	if (r) {
		dev_err(rdev->dev, "VCE init error (%d).\n", r);
		rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
		rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;
	}


To:
	r = uvd_v2_2_resume(rdev);
	if (r)
		goto error;
	r = radeon_fence_driver_start_ring(rdev, R600_RING_TYPE_UVD_INDEX);
	if (r)
		goto error_uvd;
	r = radeon_vce_resume(rdev);
	if (r)
		goto error_uvd;
	r = vce_v1_0_resume(rdev);
	if (r)
		goto error_vce;
	r = radeon_fence_driver_start_ring(rdev, TN_RING_TYPE_VCE1_INDEX);
	if (r)
		goto error_vce;
	r = radeon_fence_driver_start_ring(rdev, TN_RING_TYPE_VCE2_INDEX);
	if (r)
		goto error_vce;
	return;
error_vce:
	radeon_vce_suspend(rdev);
error_uvd:
	radeon_uvd_suspend(rdev);
error:
	dev_err(rdev->dev, "UVD/VCE startup error (%d).\n", r);
	/* On error just disable everything. */
	radeon_vce_fini(rdev);
	radeon_uvd_fini(rdev);
	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
	rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
	rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;


Is lot more clear to me than bunch of intertwine if/else. A clear error path
for which you do not have to jump through if level to see what get executed
or not on error. The only difference is that it does tie uvd and vce together.
I did that on purpose because on the hw i am playing with the vce seems to be
useless when the uvd block fails (opposite seems to be true too). If you think
we should still try to init vce when uvd fails or uvd when vce fails i can
split uvd and vce.

The other difference with existing code is that i free resources normaly use
uvd/vce on error (free fw buffer). This is just me trying to free resource
early and it has no impact as block are not working.

-----------------------------------------------------------------------------------

Second part we go from:
	if (rdev->has_uvd) {
		ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
		if (ring->ring_size) {
			r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
					     RADEON_CP_PACKET2);
			if (!r)
				r = uvd_v1_0_init(rdev);
			if (r)
				DRM_ERROR("radeon: failed initializing UVD (%d).\n", r);
		}
	}
	r = -ENOENT;
	ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
	if (ring->ring_size)
		r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
				     VCE_CMD_NO_OP);
	ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
	if (ring->ring_size)
		r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
				     VCE_CMD_NO_OP);
	if (!r)
		r = vce_v1_0_init(rdev);
	else if (r != -ENOENT)
		DRM_ERROR("radeon: failed initializing VCE (%d).\n", r);


To:
	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, RADEON_CP_PACKET2);
	if (r)
		goto error;
	r = uvd_v1_0_init(rdev);
	if (r)
		goto error_uvd;
	ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, VCE_CMD_NO_OP);
	if (r)
		goto error_vce1;
	ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, VCE_CMD_NO_OP);
	if (r)
		goto error_vce2;
	r = vce_v1_0_init(rdev);
	if (r)
		goto error_vce;
	return;
error_vce:
	radeon_ring_fini(rdev, &rdev->ring[TN_RING_TYPE_VCE2_INDEX]);
error_vce2:
	radeon_ring_fini(rdev, &rdev->ring[TN_RING_TYPE_VCE1_INDEX]);
error_vce1:
	uvd_v1_0_fini(rdev);
error_uvd:
	radeon_ring_fini(rdev, &rdev->ring[R600_RING_TYPE_UVD_INDEX]);
error:
	dev_err(rdev->dev, "UVD/VCE resume error (%d).\n", r);
	/* On error just disable everything. */
	radeon_uvd_suspend(rdev);
	radeon_vce_suspend(rdev);
	radeon_uvd_fini(rdev);
	radeon_vce_fini(rdev);
	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
	rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
	rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;

Again lot simpler to follow control flow than to jump through various level
of if/else. Again uvd and vce tied together (and again i can untie them if
you think it is better to untie them).

But this time the extra thing is that i properly disable ring if any error
happens while existing code does not.


I am not pasting the init path but it the same logic, tying uvd and vce
together and simplifying error code path.



> 
> Please also note that VCE/UVD has dependencies on power management, so that
> when they are once initialized they should NOT be turned off again.
>
> I only briefly skimmed over your patch, but it actually looks like to me
> that you broken that by trying to cleanup the initialization routine.

I have seen that but assuming Heisenbergs does not get involve, then given
that the block is not responding to register write it is unlikely that thing
will we worse if we try to disable the block. And from my testing it does
not impact power management. My guess is that the block keep reporting it is
busy and that power gating and clock gating are inhibited by that.

The other thing i am doing over existing code is freeing memory for the fw
buffer. I do not think it is a big deal. I am doing that because then i just
flag the uvd has dead (rdev->has_uvd = 0) and avoid to try to restore it
for next suspend/resume cycle or hibernation cycle.

So again the only thing i am change is the case where thing does not work.
With that patch i can actualy hibernate laptop and get back a working desktop
module video decoding/encoding no longer working. I call that an improvement.

Jérôme

Christian König March 16, 2016, 5:06 p.m. UTC | #5

Am 16.03.2016 um 16:56 schrieb Jerome Glisse:
> On Wed, Mar 16, 2016 at 04:19:16PM +0100, Christian König wrote:
>> Am 16.03.2016 um 15:59 schrieb Jerome Glisse:
>>> On Wed, Mar 16, 2016 at 2:03 PM, Christian König
>>> <[e-mail]> wrote:
>>>> Am 16.03.2016 um 13:48 schrieb Jérôme Glisse:
>>>>> From: Jérome Glisse <[e-mail]>
>>>>>
>>>>> This consolidate uvd/vce into a common shape for all generation. It
>>>>> also leverage the rdev->has_uvd flags to know what it is useless to
>>>>> try to resume/suspend uvd/vce block.
>>>>>
>>>>> There is no functional changes when there is no error. On error the
>>>>> device driver will behave differently than before after this patch.
>>>>> It should now safely ignore uvd/vce errors and keeps normal operation
>>>>> of others engine. This is an improvement over current situation where
>>>>> we have different behavior depending on GPU generation and on what
>>>>> fails.
>>>>>
>>>>> Finaly this is a preparatory step for a patch which allow to disable
>>>>> uvd/vce as a driver option.
>>>>>
>>>>> This have only been tested on southern island so please test it on
>>>>> other generations (i do not have hardware handy for now).
>>>>>
>>>>> Signed-off-by: Jérôme Glisse <[e-mail]>
>>>>> Cc: Alex Deucher <[e-mail]>
>>>>> Cc: Christian König <[e-mail]>
>>>> NAK, skipping UVD and VCE suspend/resume when initialization fails should
>>>> already be implemented.
>>>>
>>>> There might be some (quite some) bugs in there, but that doesn't justify
>>>> reworking the initialization over all different generations. Especially
>>>> since you don't have hardware to test all of them.
>>>>
>>>> Just make sure that radeon_uvd/vce_fini() is called when something goes
>>>> wrong and/or that the UVD/VCE BO is properly released.
>>>>
>>>> Regards,
>>>> Christian.
>>> Current code is a mess when it comes to handling error related to
>>> uvd/vce. This patch consolidate control flow in something easy to
>>> follow. You can check that there is absolulety no control flow change
>>> for the case where uvd/vce works and thus it does not break anything
>>> that works. It will only gracefully fails and cleanup if things go
>>> wrong. So while i have not tested on other hw i am confident that this
>>> does not introduce regression.
>>>
>>> I tried to do it without consolidation but it ended up in adding even
>>> more if() levels that line did begins after 80colums. So please
>>> reconsider because this is an improvement over existing code.
>> Well then please point out at the example of the SI or CIK code what exactly
>> is missing here.
> Going from :
> 	if (rdev->has_uvd) {
> 		r = uvd_v2_2_resume(rdev);
> 		if (!r) {
> 			r = radeon_fence_driver_start_ring(rdev,
> 							   R600_RING_TYPE_UVD_INDEX);
> 			if (r)
> 				dev_err(rdev->dev, "UVD fences init error (%d).\n", r);
> 		}
> 		if (r)
> 			rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
> 	}
> 	r = radeon_vce_resume(rdev);
> 	if (!r) {
> 		r = vce_v1_0_resume(rdev);
> 		if (!r)
> 			r = radeon_fence_driver_start_ring(rdev,
> 							   TN_RING_TYPE_VCE1_INDEX);
> 		if (!r)
> 			r = radeon_fence_driver_start_ring(rdev,
> 							   TN_RING_TYPE_VCE2_INDEX);
> 	}
> 	if (r) {
> 		dev_err(rdev->dev, "VCE init error (%d).\n", r);
> 		rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
> 		rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;
> 	}
>
>
> To:
> 	r = uvd_v2_2_resume(rdev);
> 	if (r)
> 		goto error;
> 	r = radeon_fence_driver_start_ring(rdev, R600_RING_TYPE_UVD_INDEX);
> 	if (r)
> 		goto error_uvd;
> 	r = radeon_vce_resume(rdev);
> 	if (r)
> 		goto error_uvd;
> 	r = vce_v1_0_resume(rdev);
> 	if (r)
> 		goto error_vce;
> 	r = radeon_fence_driver_start_ring(rdev, TN_RING_TYPE_VCE1_INDEX);
> 	if (r)
> 		goto error_vce;
> 	r = radeon_fence_driver_start_ring(rdev, TN_RING_TYPE_VCE2_INDEX);
> 	if (r)
> 		goto error_vce;
> 	return;
> error_vce:
> 	radeon_vce_suspend(rdev);
> error_uvd:
> 	radeon_uvd_suspend(rdev);
> error:
> 	dev_err(rdev->dev, "UVD/VCE startup error (%d).\n", r);
> 	/* On error just disable everything. */
> 	radeon_vce_fini(rdev);
> 	radeon_uvd_fini(rdev);
> 	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
> 	rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
> 	rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;

And as I said that is exactly what you should NOT be doing here. Once 
the firmware is loaded the block should be kept in that state.

Freeing the memory allocated for the firmware is also not a good idea at 
all because we don't know who exactly is accessing it.

>
>
> Is lot more clear to me than bunch of intertwine if/else. A clear error path
> for which you do not have to jump through if level to see what get executed
> or not on error. The only difference is that it does tie uvd and vce together.
> I did that on purpose because on the hw i am playing with the vce seems to be
> useless when the uvd block fails (opposite seems to be true too). If you think
> we should still try to init vce when uvd fails or uvd when vce fails i can
> split uvd and vce.

UVD and VCE are two completely separate blocks, they shouldn't be 
related to each other in anyway.

When you see failures of both at the same time it's rather unlikely that 
it is actually related to them.

>
> The other difference with existing code is that i free resources normaly use
> uvd/vce on error (free fw buffer). This is just me trying to free resource
> early and it has no impact as block are not working.
>
> -----------------------------------------------------------------------------------
>
> Second part we go from:
> 	if (rdev->has_uvd) {
> 		ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
> 		if (ring->ring_size) {
> 			r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
> 					     RADEON_CP_PACKET2);
> 			if (!r)
> 				r = uvd_v1_0_init(rdev);
> 			if (r)
> 				DRM_ERROR("radeon: failed initializing UVD (%d).\n", r);
> 		}
> 	}
> 	r = -ENOENT;
> 	ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
> 	if (ring->ring_size)
> 		r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
> 				     VCE_CMD_NO_OP);
> 	ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
> 	if (ring->ring_size)
> 		r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
> 				     VCE_CMD_NO_OP);
> 	if (!r)
> 		r = vce_v1_0_init(rdev);
> 	else if (r != -ENOENT)
> 		DRM_ERROR("radeon: failed initializing VCE (%d).\n", r);
>
>
> To:
> 	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
> 	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, RADEON_CP_PACKET2);
> 	if (r)
> 		goto error;
> 	r = uvd_v1_0_init(rdev);
> 	if (r)
> 		goto error_uvd;
> 	ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
> 	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, VCE_CMD_NO_OP);
> 	if (r)
> 		goto error_vce1;
> 	ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
> 	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, VCE_CMD_NO_OP);
> 	if (r)
> 		goto error_vce2;
> 	r = vce_v1_0_init(rdev);
> 	if (r)
> 		goto error_vce;
> 	return;
> error_vce:
> 	radeon_ring_fini(rdev, &rdev->ring[TN_RING_TYPE_VCE2_INDEX]);
> error_vce2:
> 	radeon_ring_fini(rdev, &rdev->ring[TN_RING_TYPE_VCE1_INDEX]);
> error_vce1:
> 	uvd_v1_0_fini(rdev);
> error_uvd:
> 	radeon_ring_fini(rdev, &rdev->ring[R600_RING_TYPE_UVD_INDEX]);
> error:
> 	dev_err(rdev->dev, "UVD/VCE resume error (%d).\n", r);
> 	/* On error just disable everything. */
> 	radeon_uvd_suspend(rdev);
> 	radeon_vce_suspend(rdev);
> 	radeon_uvd_fini(rdev);
> 	radeon_vce_fini(rdev);
> 	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
> 	rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
> 	rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;
>
> Again lot simpler to follow control flow than to jump through various level
> of if/else. Again uvd and vce tied together (and again i can untie them if
> you think it is better to untie them).
>
> But this time the extra thing is that i properly disable ring if any error
> happens while existing code does not.

And again that is exactly what we should NOT do.

When initialization fails we don't know in which state the ring buffer 
and micro engines are, so freeing them and giving the space back to be 
reused in clearly not a good idea.

All we should do is clearing the ready flag when something fails to 
prevent userspace from making command submissions to the failed engine.

>
>
> I am not pasting the init path but it the same logic, tying uvd and vce
> together and simplifying error code path.
>
>
>
>> Please also note that VCE/UVD has dependencies on power management, so that
>> when they are once initialized they should NOT be turned off again.
>>
>> I only briefly skimmed over your patch, but it actually looks like to me
>> that you broken that by trying to cleanup the initialization routine.
> I have seen that but assuming Heisenbergs does not get involve, then given
> that the block is not responding to register write it is unlikely that thing
> will we worse if we try to disable the block. And from my testing it does
> not impact power management. My guess is that the block keep reporting it is
> busy and that power gating and clock gating are inhibited by that.

It's more complicated than that just a simple busy signal. The engines 
actively communicate with the power management controller to tell them 
their needs and limits for the clocks based on the workload they have.

Once initialized the power management controller expects the UVD and VCE 
micro-controllers to answer such requests.

Failing to do so can get you stuck at a specific power level.

>
> The other thing i am doing over existing code is freeing memory for the fw
> buffer. I do not think it is a big deal. I am doing that because then i just
> flag the uvd has dead (rdev->has_uvd = 0) and avoid to try to restore it
> for next suspend/resume cycle or hibernation cycle.
>
> So again the only thing i am change is the case where thing does not work.
> With that patch i can actualy hibernate laptop and get back a working desktop
> module video decoding/encoding no longer working. I call that an improvement.

It's nice that it works for you now, but my laptop is working fine with 
UVD and VCE as well and I would like to keep it that way.

As far as I can see you're actually messing the error handling up quite 
a bit here instead of improving it.

So please describe in detail what the problems you are seeing and why 
disabling both UVD and VCE helps with them.

A kernel log from a failed suspend/resume cycle would help quite a bit here.

Regards,
Christian.

>
> Jérôme

Jerome Glisse March 16, 2016, 5:43 p.m. UTC | #6

On Wed, Mar 16, 2016 at 06:06:36PM +0100, Christian König wrote:
> Am 16.03.2016 um 16:56 schrieb Jerome Glisse:
> >On Wed, Mar 16, 2016 at 04:19:16PM +0100, Christian König wrote:
> >>Am 16.03.2016 um 15:59 schrieb Jerome Glisse:
> >>>On Wed, Mar 16, 2016 at 2:03 PM, Christian König
> >>><[e-mail]> wrote:
> >>>>Am 16.03.2016 um 13:48 schrieb Jérôme Glisse:
> >>>>>From: Jérome Glisse <[e-mail]>
> >>>>>
> >>>>>This consolidate uvd/vce into a common shape for all generation. It
> >>>>>also leverage the rdev->has_uvd flags to know what it is useless to
> >>>>>try to resume/suspend uvd/vce block.
> >>>>>
> >>>>>There is no functional changes when there is no error. On error the
> >>>>>device driver will behave differently than before after this patch.
> >>>>>It should now safely ignore uvd/vce errors and keeps normal operation
> >>>>>of others engine. This is an improvement over current situation where
> >>>>>we have different behavior depending on GPU generation and on what
> >>>>>fails.
> >>>>>
> >>>>>Finaly this is a preparatory step for a patch which allow to disable
> >>>>>uvd/vce as a driver option.
> >>>>>
> >>>>>This have only been tested on southern island so please test it on
> >>>>>other generations (i do not have hardware handy for now).
> >>>>>
> >>>>>Signed-off-by: Jérôme Glisse <[e-mail]>
> >>>>>Cc: Alex Deucher <[e-mail]>
> >>>>>Cc: Christian König <[e-mail]>
> >>>>NAK, skipping UVD and VCE suspend/resume when initialization fails should
> >>>>already be implemented.
> >>>>
> >>>>There might be some (quite some) bugs in there, but that doesn't justify
> >>>>reworking the initialization over all different generations. Especially
> >>>>since you don't have hardware to test all of them.
> >>>>
> >>>>Just make sure that radeon_uvd/vce_fini() is called when something goes
> >>>>wrong and/or that the UVD/VCE BO is properly released.
> >>>>
> >>>>Regards,
> >>>>Christian.
> >>>Current code is a mess when it comes to handling error related to
> >>>uvd/vce. This patch consolidate control flow in something easy to
> >>>follow. You can check that there is absolulety no control flow change
> >>>for the case where uvd/vce works and thus it does not break anything
> >>>that works. It will only gracefully fails and cleanup if things go
> >>>wrong. So while i have not tested on other hw i am confident that this
> >>>does not introduce regression.
> >>>
> >>>I tried to do it without consolidation but it ended up in adding even
> >>>more if() levels that line did begins after 80colums. So please
> >>>reconsider because this is an improvement over existing code.
> >>Well then please point out at the example of the SI or CIK code what exactly
> >>is missing here.
> >Going from :
> >	if (rdev->has_uvd) {
> >		r = uvd_v2_2_resume(rdev);
> >		if (!r) {
> >			r = radeon_fence_driver_start_ring(rdev,
> >							   R600_RING_TYPE_UVD_INDEX);
> >			if (r)
> >				dev_err(rdev->dev, "UVD fences init error (%d).\n", r);
> >		}
> >		if (r)
> >			rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
> >	}
> >	r = radeon_vce_resume(rdev);
> >	if (!r) {
> >		r = vce_v1_0_resume(rdev);
> >		if (!r)
> >			r = radeon_fence_driver_start_ring(rdev,
> >							   TN_RING_TYPE_VCE1_INDEX);
> >		if (!r)
> >			r = radeon_fence_driver_start_ring(rdev,
> >							   TN_RING_TYPE_VCE2_INDEX);
> >	}
> >	if (r) {
> >		dev_err(rdev->dev, "VCE init error (%d).\n", r);
> >		rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
> >		rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;
> >	}
> >
> >
> >To:
> >	r = uvd_v2_2_resume(rdev);
> >	if (r)
> >		goto error;
> >	r = radeon_fence_driver_start_ring(rdev, R600_RING_TYPE_UVD_INDEX);
> >	if (r)
> >		goto error_uvd;
> >	r = radeon_vce_resume(rdev);
> >	if (r)
> >		goto error_uvd;
> >	r = vce_v1_0_resume(rdev);
> >	if (r)
> >		goto error_vce;
> >	r = radeon_fence_driver_start_ring(rdev, TN_RING_TYPE_VCE1_INDEX);
> >	if (r)
> >		goto error_vce;
> >	r = radeon_fence_driver_start_ring(rdev, TN_RING_TYPE_VCE2_INDEX);
> >	if (r)
> >		goto error_vce;
> >	return;
> >error_vce:
> >	radeon_vce_suspend(rdev);
> >error_uvd:
> >	radeon_uvd_suspend(rdev);
> >error:
> >	dev_err(rdev->dev, "UVD/VCE startup error (%d).\n", r);
> >	/* On error just disable everything. */
> >	radeon_vce_fini(rdev);
> >	radeon_uvd_fini(rdev);
> >	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
> >	rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
> >	rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;
> 
> And as I said that is exactly what you should NOT be doing here. Once the
> firmware is loaded the block should be kept in that state.
> 
> Freeing the memory allocated for the firmware is also not a good idea at all
> because we don't know who exactly is accessing it.

And what i am saying is that block is not responding so it does not matter
what memory endup there as it only try to read from the firmware afaict.
So nothing bad will come from that, block is already in non functional
state what worse can happen ?


> >Is lot more clear to me than bunch of intertwine if/else. A clear error path
> >for which you do not have to jump through if level to see what get executed
> >or not on error. The only difference is that it does tie uvd and vce together.
> >I did that on purpose because on the hw i am playing with the vce seems to be
> >useless when the uvd block fails (opposite seems to be true too). If you think
> >we should still try to init vce when uvd fails or uvd when vce fails i can
> >split uvd and vce.
> 
> UVD and VCE are two completely separate blocks, they shouldn't be related to
> each other in anyway.
> 
> When you see failures of both at the same time it's rather unlikely that it
> is actually related to them.

Fine i will split then.


> >The other difference with existing code is that i free resources normaly use
> >uvd/vce on error (free fw buffer). This is just me trying to free resource
> >early and it has no impact as block are not working.
> >
> >-----------------------------------------------------------------------------------
> >
> >Second part we go from:
> >	if (rdev->has_uvd) {
> >		ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
> >		if (ring->ring_size) {
> >			r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
> >					     RADEON_CP_PACKET2);
> >			if (!r)
> >				r = uvd_v1_0_init(rdev);
> >			if (r)
> >				DRM_ERROR("radeon: failed initializing UVD (%d).\n", r);
> >		}
> >	}
> >	r = -ENOENT;
> >	ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
> >	if (ring->ring_size)
> >		r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
> >				     VCE_CMD_NO_OP);
> >	ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
> >	if (ring->ring_size)
> >		r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
> >				     VCE_CMD_NO_OP);
> >	if (!r)
> >		r = vce_v1_0_init(rdev);
> >	else if (r != -ENOENT)
> >		DRM_ERROR("radeon: failed initializing VCE (%d).\n", r);
> >
> >
> >To:
> >	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
> >	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, RADEON_CP_PACKET2);
> >	if (r)
> >		goto error;
> >	r = uvd_v1_0_init(rdev);
> >	if (r)
> >		goto error_uvd;
> >	ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
> >	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, VCE_CMD_NO_OP);
> >	if (r)
> >		goto error_vce1;
> >	ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
> >	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, VCE_CMD_NO_OP);
> >	if (r)
> >		goto error_vce2;
> >	r = vce_v1_0_init(rdev);
> >	if (r)
> >		goto error_vce;
> >	return;
> >error_vce:
> >	radeon_ring_fini(rdev, &rdev->ring[TN_RING_TYPE_VCE2_INDEX]);
> >error_vce2:
> >	radeon_ring_fini(rdev, &rdev->ring[TN_RING_TYPE_VCE1_INDEX]);
> >error_vce1:
> >	uvd_v1_0_fini(rdev);
> >error_uvd:
> >	radeon_ring_fini(rdev, &rdev->ring[R600_RING_TYPE_UVD_INDEX]);
> >error:
> >	dev_err(rdev->dev, "UVD/VCE resume error (%d).\n", r);
> >	/* On error just disable everything. */
> >	radeon_uvd_suspend(rdev);
> >	radeon_vce_suspend(rdev);
> >	radeon_uvd_fini(rdev);
> >	radeon_vce_fini(rdev);
> >	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
> >	rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
> >	rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;
> >
> >Again lot simpler to follow control flow than to jump through various level
> >of if/else. Again uvd and vce tied together (and again i can untie them if
> >you think it is better to untie them).
> >
> >But this time the extra thing is that i properly disable ring if any error
> >happens while existing code does not.
> 
> And again that is exactly what we should NOT do.
> 
> When initialization fails we don't know in which state the ring buffer and
> micro engines are, so freeing them and giving the space back to be reused in
> clearly not a good idea.
> 
> All we should do is clearing the ready flag when something fails to prevent
> userspace from making command submissions to the failed engine.

This block only read from that memory ! Never write (unless it modify the fw
in place which would surprise me). So when block is in bad states it does not
matter what endup there, block is not processing anything.

> >I am not pasting the init path but it the same logic, tying uvd and vce
> >together and simplifying error code path.
> >
> >
> >
> >>Please also note that VCE/UVD has dependencies on power management, so that
> >>when they are once initialized they should NOT be turned off again.
> >>
> >>I only briefly skimmed over your patch, but it actually looks like to me
> >>that you broken that by trying to cleanup the initialization routine.
> >I have seen that but assuming Heisenbergs does not get involve, then given
> >that the block is not responding to register write it is unlikely that thing
> >will we worse if we try to disable the block. And from my testing it does
> >not impact power management. My guess is that the block keep reporting it is
> >busy and that power gating and clock gating are inhibited by that.
> 
> It's more complicated than that just a simple busy signal. The engines
> actively communicate with the power management controller to tell them their
> needs and limits for the clocks based on the workload they have.
> 
> Once initialized the power management controller expects the UVD and VCE
> micro-controllers to answer such requests.
> 
> Failing to do so can get you stuck at a specific power level.

Yes and what i am saying is that block is in bad states so either it is
answering always busy and thus stop power level change or it is not answering
and power level can change. So trying to disable the block leads to the
exact same situation, either the block shutdown and power level can change
or it does not and thing are exactly as if we did nothing !


> >The other thing i am doing over existing code is freeing memory for the fw
> >buffer. I do not think it is a big deal. I am doing that because then i just
> >flag the uvd has dead (rdev->has_uvd = 0) and avoid to try to restore it
> >for next suspend/resume cycle or hibernation cycle.
> >
> >So again the only thing i am change is the case where thing does not work.
> >With that patch i can actualy hibernate laptop and get back a working desktop
> >module video decoding/encoding no longer working. I call that an improvement.
> 
> It's nice that it works for you now, but my laptop is working fine with UVD
> and VCE as well and I would like to keep it that way.

And my patch will not break that, test it if you do not believe me !


> As far as I can see you're actually messing the error handling up quite a
> bit here instead of improving it.

I am not ! I AM MAKING IT CLEAR WHAT HAPPENS. YES ON ERROR I TRY TO DISABLE
THING BUT IF BLOCK IS NOT ANSWERING THIS CAN NOT MAKE THING WORSE.


> So please describe in detail what the problems you are seeing and why
> disabling both UVD and VCE helps with them.

I said it above already, on second part when we fails uvd/vce we do not
disable ring and things goes from bad to worse.

> A kernel log from a failed suspend/resume cycle would help quite a bit here.

[   62.111042] [drm] ring test on 5 succeeded in 2 usecs
[   62.111046] [drm] UVD initialized successfully.
[   62.111080] [drm] ib test on ring 0 succeeded in 0 usecs
[   62.111110] [drm] ib test on ring 1 succeeded in 0 usecs
[   62.111137] [drm] ib test on ring 2 succeeded in 0 usecs
[   62.111198] [drm] ib test on ring 3 succeeded in 0 usecs
[   62.111257] [drm] ib test on ring 4 succeeded in 0 usecs
[   62.309312] usb 1-7: reset high-speed USB device number 3 using xhci_hcd
[   62.362819] ACPI: \_SB_.PCI0.LPCB.SIO_.COM1: ACPI_NOTIFY_BUS_CHECK event
[   62.362824] ACPI: \_SB_.PCI0.RP05: ACPI_NOTIFY_BUS_CHECK event
[   62.362825] ACPI: \_SB_.PCI0.EHC1: ACPI_NOTIFY_BUS_CHECK event
[   62.362827] ACPI: \_SB_.PCI0.EHC2: ACPI_NOTIFY_BUS_CHECK event
[   62.626546] usb 1-12: reset full-speed USB device number 4 using xhci_hcd
[   62.793373] parport_pc 00:08: activated
[   62.858604] tpm_tis 00:0c: TPM is disabled/deactivated (0x7)
[   63.625310] psmouse serio2: alps: Unknown ALPS touchpad: E7=10 00 64, EC=10 00 64
[   64.826916] psmouse serio3: synaptics: queried max coordinates: x [..5660], y [..4730]
[   64.858652] psmouse serio3: synaptics: queried min coordinates: x [1324..], y [1248..]
[   72.288487] radeon 0000:01:00.0: ring 5 stalled for more than 10020msec
[   72.288489] radeon 0000:01:00.0: GPU lockup (current fence id 0x0000000000000002 last fence id 0x0000000000000004 on ring 5)
[   72.288577] [drm:uvd_v1_0_ib_test [radeon]] *ERROR* radeon: fence wait failed (-35).
[   72.288594] [drm:radeon_ib_ring_tests [radeon]] *ERROR* radeon: failed testing IB on ring 5 (-35).
[   72.473034] [drm:si_dpm_set_power_state [radeon]] *ERROR* si_set_sw_state failed
[   73.452173] [drm:radeon_dp_link_train [radeon]] *ERROR* displayport link status failed
[   73.452197] [drm:radeon_dp_link_train [radeon]] *ERROR* clock recovery failed
[   74.296728] [drm:radeon_dp_link_train [radeon]] *ERROR* displayport link status failed
[   74.296744] [drm:radeon_dp_link_train [radeon]] *ERROR* clock recovery failed


So first par of uvd succeed but vce_v1_0_resume() fails and after that uvd fails too
(vce too). I doubt you can fix that until you can fix the firmware signature check
issue. What happens is that on resume from hibernation the first kernel load radeon
and initialize the GPU (this work properly and VCE block is initialized properly).

But then it resume the hibernated kernel (kexec) and the radeon resume code path by
reprogramming the memory configuration of the GPU make the fw unaccessible or at
different address compare to first boot kernel and thing from that point the vce
block is in bad state (impacting the uvd block afaict). This trickle down to other
block like displayport and you endup with no display.

Jérôme

Christian König March 16, 2016, 6:51 p.m. UTC | #7

Am 16.03.2016 um 18:43 schrieb Jerome Glisse:
> On Wed, Mar 16, 2016 at 06:06:36PM +0100, Christian König wrote:
>> [SNIP]
>> And as I said that is exactly what you should NOT be doing here. Once the
>> firmware is loaded the block should be kept in that state.
>>
>> Freeing the memory allocated for the firmware is also not a good idea at all
>> because we don't know who exactly is accessing it.
> And what i am saying is that block is not responding so it does not matter
> what memory endup there as it only try to read from the firmware afaict.
> So nothing bad will come from that, block is already in non functional
> state what worse can happen ?

That the block is not responding to CPU register accesses just means 
that the SRBM read side interface doesn't work for some reason.

Depending on what the cause is the micro engine usually still happily 
read and write memory when that happens.

When the whole engine is hung you usually don't get working power 
management as well.

>>> [SNIP]
>>>
>>> Again lot simpler to follow control flow than to jump through various level
>>> of if/else. Again uvd and vce tied together (and again i can untie them if
>>> you think it is better to untie them).
>>>
>>> But this time the extra thing is that i properly disable ring if any error
>>> happens while existing code does not.
>> And again that is exactly what we should NOT do.
>>
>> When initialization fails we don't know in which state the ring buffer and
>> micro engines are, so freeing them and giving the space back to be reused in
>> clearly not a good idea.
>>
>> All we should do is clearing the ready flag when something fails to prevent
>> userspace from making command submissions to the failed engine.
> This block only read from that memory ! Never write (unless it modify the fw
> in place which would surprise me). So when block is in bad states it does not
> matter what endup there, block is not processing anything.

No, the memory is quite heavily written as well. Only BAR0 is code, BAR1 
is the heap and BAR2 the stack of the micro engine.

Those are usually heavily accessed by the interrupt handlers even when 
the engine is completely stuck otherwise.

The only way I know to prevent that is to stall the both the register as 
well as the memory bus, wait for a moment and then put the micro engine 
into soft reset. See uvd_v1_0_stop() how that is done.

But as I said after that you can usually forget power management as well.

>>> I am not pasting the init path but it the same logic, tying uvd and vce
>>> together and simplifying error code path.
>>>
>>>
>>>
>>>> Please also note that VCE/UVD has dependencies on power management, so that
>>>> when they are once initialized they should NOT be turned off again.
>>>>
>>>> I only briefly skimmed over your patch, but it actually looks like to me
>>>> that you broken that by trying to cleanup the initialization routine.
>>> I have seen that but assuming Heisenbergs does not get involve, then given
>>> that the block is not responding to register write it is unlikely that thing
>>> will we worse if we try to disable the block. And from my testing it does
>>> not impact power management. My guess is that the block keep reporting it is
>>> busy and that power gating and clock gating are inhibited by that.
>> It's more complicated than that just a simple busy signal. The engines
>> actively communicate with the power management controller to tell them their
>> needs and limits for the clocks based on the workload they have.
>>
>> Once initialized the power management controller expects the UVD and VCE
>> micro-controllers to answer such requests.
>>
>> Failing to do so can get you stuck at a specific power level.
> Yes and what i am saying is that block is in bad states so either it is
> answering always busy and thus stop power level change or it is not answering
> and power level can change. So trying to disable the block leads to the
> exact same situation, either the block shutdown and power level can change
> or it does not and thing are exactly as if we did nothing !

I've tested that multiple times and as long as you don't turn of the 
VCPU completely it's usually still answering the interrupt handler even 
when it is stuck in an endless loop otherwise and not doing anything else.

That's also the reason why it is so dangerous to just put the block into 
reset in this state or touch it otherwise.

You can't stall it because the SRBM interface is borked and you don't 
have access to the registers any more and you can't properly reset it 
either because when you do that in the middle of a memory transaction 
the whole system goes down.

[SNIP]
>> As far as I can see you're actually messing the error handling up quite a
>> bit here instead of improving it.
> I am not ! I AM MAKING IT CLEAR WHAT HAPPENS. YES ON ERROR I TRY TO DISABLE
> THING BUT IF BLOCK IS NOT ANSWERING THIS CAN NOT MAKE THING WORSE.

Calm down, I'm just trying to explain here how the hardware works and 
why your approach can cause trouble as well.

I'm perfectly fine with the cleanups on the control flow and the coding 
style as long as we can properly test them on at least some hardware 
generations.

>> So please describe in detail what the problems you are seeing and why
>> disabling both UVD and VCE helps with them.
> I said it above already, on second part when we fails uvd/vce we do not
> disable ring and things goes from bad to worse.
>
>> A kernel log from a failed suspend/resume cycle would help quite a bit here.
> [   62.111042] [drm] ring test on 5 succeeded in 2 usecs
> [   62.111046] [drm] UVD initialized successfully.
> [   62.111080] [drm] ib test on ring 0 succeeded in 0 usecs
> [   62.111110] [drm] ib test on ring 1 succeeded in 0 usecs
> [   62.111137] [drm] ib test on ring 2 succeeded in 0 usecs
> [   62.111198] [drm] ib test on ring 3 succeeded in 0 usecs
> [   62.111257] [drm] ib test on ring 4 succeeded in 0 usecs
> [   62.309312] usb 1-7: reset high-speed USB device number 3 using xhci_hcd
> [   62.362819] ACPI: \_SB_.PCI0.LPCB.SIO_.COM1: ACPI_NOTIFY_BUS_CHECK event
> [   62.362824] ACPI: \_SB_.PCI0.RP05: ACPI_NOTIFY_BUS_CHECK event
> [   62.362825] ACPI: \_SB_.PCI0.EHC1: ACPI_NOTIFY_BUS_CHECK event
> [   62.362827] ACPI: \_SB_.PCI0.EHC2: ACPI_NOTIFY_BUS_CHECK event
> [   62.626546] usb 1-12: reset full-speed USB device number 4 using xhci_hcd
> [   62.793373] parport_pc 00:08: activated
> [   62.858604] tpm_tis 00:0c: TPM is disabled/deactivated (0x7)
> [   63.625310] psmouse serio2: alps: Unknown ALPS touchpad: E7=10 00 64, EC=10 00 64
> [   64.826916] psmouse serio3: synaptics: queried max coordinates: x [..5660], y [..4730]
> [   64.858652] psmouse serio3: synaptics: queried min coordinates: x [1324..], y [1248..]
> [   72.288487] radeon 0000:01:00.0: ring 5 stalled for more than 10020msec
> [   72.288489] radeon 0000:01:00.0: GPU lockup (current fence id 0x0000000000000002 last fence id 0x0000000000000004 on ring 5)
> [   72.288577] [drm:uvd_v1_0_ib_test [radeon]] *ERROR* radeon: fence wait failed (-35).
> [   72.288594] [drm:radeon_ib_ring_tests [radeon]] *ERROR* radeon: failed testing IB on ring 5 (-35).
> [   72.473034] [drm:si_dpm_set_power_state [radeon]] *ERROR* si_set_sw_state failed
> [   73.452173] [drm:radeon_dp_link_train [radeon]] *ERROR* displayport link status failed
> [   73.452197] [drm:radeon_dp_link_train [radeon]] *ERROR* clock recovery failed
> [   74.296728] [drm:radeon_dp_link_train [radeon]] *ERROR* displayport link status failed
> [   74.296744] [drm:radeon_dp_link_train [radeon]] *ERROR* clock recovery failed
>
>
> So first par of uvd succeed but vce_v1_0_resume() fails and after that uvd fails too
> (vce too). I doubt you can fix that until you can fix the firmware signature check
> issue. What happens is that on resume from hibernation the first kernel load radeon
> and initialize the GPU (this work properly and VCE block is initialized properly).

I'm not sure if that is actually an problem with the firmware signature 
check, but let's focus on the issue at hand first.

> But then it resume the hibernated kernel (kexec) and the radeon resume code path by
> reprogramming the memory configuration of the GPU make the fw unaccessible or at
> different address compare to first boot kernel and thing from that point the vce
> block is in bad state (impacting the uvd block afaict). This trickle down to other
> block like displayport and you endup with no display.

Think about it, that's exactly as I explained above.

The VCPU (UVD) and ECPU (VCE) keeps running after the first boot cycle, 
even when they are not accessible from the CPU any more for some reason.

Because of this the power management can still switch clocks and turn 
blocks on and off. etc... and that's why the memory clock can be raised 
to match your resolution on the screen and DP link training still works.

Now on the second cycle we somehow totally crash them and because of 
that the power management can't raise the clocks any more:
> [   72.473034] [drm:si_dpm_set_power_state [radeon]] *ERROR* si_set_sw_state failed

That results in not enough memory bandwidth or a PLL being powered down 
and so DP link training fails as well.

As a band aid I suggest to just set has_uvd to false when either of the 
vce resume or the uvd resume fails.

Also feel free to cleanup the control flow on coding style as long as 
you don't try to free the memory for the micro engines or try to touch 
the hardware blocks when something fails.

Regards,
Christian.

Dave Airlie March 16, 2016, 8:41 p.m. UTC | #8

Just an aside,

So is there no way to do hibernate with these blocks?

Like can you not cleanly shut them down without doing a power cycle.

I have to say UVD is a real pain in the ass from a stability pov, I'd
kinda wished I'd enforced AMD creating something like intel-gpu-tools
and having tests to make sure GPU reset etc stayed working before
merging it.

Dave.

Alex Deucher March 16, 2016, 8:58 p.m. UTC | #9

> -----Original Message-----

> From: Dave Airlie [mailto:[e-mail]]

> Sent: Wednesday, March 16, 2016 4:41 PM

> To: Koenig, Christian

> Cc: Jerome Glisse; Deucher, Alexander; [e-mail]

> Subject: Re: [PATCH 3/4] drm/radeon: consolidate uvd/vce initialization,

> resume and suspend.

> 

> Just an aside,

> 

> So is there no way to do hibernate with these blocks?

> 

> Like can you not cleanly shut them down without doing a power cycle.


Christian is the expert, but my understanding is that once they are running, you can't re-initialize them without cutting the power to them (either via S3/S4 or powergating the block on chips that support it).  On windows hibernate is more like suspend.  There is no additional freeze and thaw in between. Probably the proper fix is only init the blocks once on resume from hibernate instead of doing it twice.  Kexec will probably have similar problems.

Alex

Daniel Vetter March 17, 2016, 7:36 a.m. UTC | #10

On Thu, Mar 17, 2016 at 06:41:14AM +1000, Dave Airlie wrote:
> Just an aside,
> 
> So is there no way to do hibernate with these blocks?
> 
> Like can you not cleanly shut them down without doing a power cycle.
> 
> I have to say UVD is a real pain in the ass from a stability pov, I'd
> kinda wished I'd enforced AMD creating something like intel-gpu-tools
> and having tests to make sure GPU reset etc stayed working before
> merging it.

igt already supports running on any kind of drm device, and it has a bunch
of vc4 specific testcases on top. If anyone finds offence in the "intel"
part, we can rename it to igt gpu tools/tests ;-)

Cheers, Daniel

Michel Dänzer March 18, 2016, 4 a.m. UTC | #11

On 17.03.2016 16:36, Daniel Vetter wrote:
> On Thu, Mar 17, 2016 at 06:41:14AM +1000, Dave Airlie wrote:
>> Just an aside,
>>
>> So is there no way to do hibernate with these blocks?
>>
>> Like can you not cleanly shut them down without doing a power cycle.
>>
>> I have to say UVD is a real pain in the ass from a stability pov, I'd
>> kinda wished I'd enforced AMD creating something like intel-gpu-tools
>> and having tests to make sure GPU reset etc stayed working before
>> merging it.
> 
> igt already supports running on any kind of drm device, and it has a bunch
> of vc4 specific testcases on top. If anyone finds offence in the "intel"
> part, we can rename it to igt gpu tools/tests ;-)

Any tips for running the tests on non-Intel GPUs? I tried piglit igt.py,
but it was generating tens of thousands of failures from tests which
look Intel specific.

Daniel Vetter March 18, 2016, 8:16 a.m. UTC | #12

On Fri, Mar 18, 2016 at 01:00:26PM +0900, Michel Dänzer wrote:
> On 17.03.2016 16:36, Daniel Vetter wrote:
> > On Thu, Mar 17, 2016 at 06:41:14AM +1000, Dave Airlie wrote:
> >> Just an aside,
> >>
> >> So is there no way to do hibernate with these blocks?
> >>
> >> Like can you not cleanly shut them down without doing a power cycle.
> >>
> >> I have to say UVD is a real pain in the ass from a stability pov, I'd
> >> kinda wished I'd enforced AMD creating something like intel-gpu-tools
> >> and having tests to make sure GPU reset etc stayed working before
> >> merging it.
> > 
> > igt already supports running on any kind of drm device, and it has a bunch
> > of vc4 specific testcases on top. If anyone finds offence in the "intel"
> > part, we can rename it to igt gpu tools/tests ;-)
> 
> Any tips for running the tests on non-Intel GPUs? I tried piglit igt.py,
> but it was generating tens of thousands of failures from tests which
> look Intel specific.

Yeah Chris again broke the SKIP logic in gem_concurrent_blt/all testcases.
Just explicitly exclude those with -x gem_concurrent. The problem is that
hw/kernel feature checks aren't properlty encapsulated in the right
igt_fixture or igt_subtest blocks, so it falls over. Specifically the
access_mode->require() test is only protetected by
igt_only_list_subtests(), which is the wrong way to do it.

Adding Chris.
-Daniel

Daniel Vetter March 19, 2016, 9:41 a.m. UTC | #13

On Fri, Mar 18, 2016 at 09:16:08AM +0100, Daniel Vetter wrote:
> On Fri, Mar 18, 2016 at 01:00:26PM +0900, Michel Dänzer wrote:
> > On 17.03.2016 16:36, Daniel Vetter wrote:
> > > On Thu, Mar 17, 2016 at 06:41:14AM +1000, Dave Airlie wrote:
> > >> Just an aside,
> > >>
> > >> So is there no way to do hibernate with these blocks?
> > >>
> > >> Like can you not cleanly shut them down without doing a power cycle.
> > >>
> > >> I have to say UVD is a real pain in the ass from a stability pov, I'd
> > >> kinda wished I'd enforced AMD creating something like intel-gpu-tools
> > >> and having tests to make sure GPU reset etc stayed working before
> > >> merging it.
> > > 
> > > igt already supports running on any kind of drm device, and it has a bunch
> > > of vc4 specific testcases on top. If anyone finds offence in the "intel"
> > > part, we can rename it to igt gpu tools/tests ;-)
> > 
> > Any tips for running the tests on non-Intel GPUs? I tried piglit igt.py,
> > but it was generating tens of thousands of failures from tests which
> > look Intel specific.
> 
> Yeah Chris again broke the SKIP logic in gem_concurrent_blt/all testcases.
> Just explicitly exclude those with -x gem_concurrent. The problem is that
> hw/kernel feature checks aren't properlty encapsulated in the right
> igt_fixture or igt_subtest blocks, so it falls over. Specifically the
> access_mode->require() test is only protetected by
> igt_only_list_subtests(), which is the wrong way to do it.
> 
> Adding Chris.

Ok, fixed pushed now using the just added igt_subtest_group blocks. Please
scream when anything else falls apart.
-Daniel

Daniel Vetter March 19, 2016, 10:46 a.m. UTC | #14

On Sat, Mar 19, 2016 at 10:41:59AM +0100, Daniel Vetter wrote:
> On Fri, Mar 18, 2016 at 09:16:08AM +0100, Daniel Vetter wrote:
> > On Fri, Mar 18, 2016 at 01:00:26PM +0900, Michel Dänzer wrote:
> > > On 17.03.2016 16:36, Daniel Vetter wrote:
> > > > On Thu, Mar 17, 2016 at 06:41:14AM +1000, Dave Airlie wrote:
> > > >> Just an aside,
> > > >>
> > > >> So is there no way to do hibernate with these blocks?
> > > >>
> > > >> Like can you not cleanly shut them down without doing a power cycle.
> > > >>
> > > >> I have to say UVD is a real pain in the ass from a stability pov, I'd
> > > >> kinda wished I'd enforced AMD creating something like intel-gpu-tools
> > > >> and having tests to make sure GPU reset etc stayed working before
> > > >> merging it.
> > > > 
> > > > igt already supports running on any kind of drm device, and it has a bunch
> > > > of vc4 specific testcases on top. If anyone finds offence in the "intel"
> > > > part, we can rename it to igt gpu tools/tests ;-)
> > > 
> > > Any tips for running the tests on non-Intel GPUs? I tried piglit igt.py,
> > > but it was generating tens of thousands of failures from tests which
> > > look Intel specific.
> > 
> > Yeah Chris again broke the SKIP logic in gem_concurrent_blt/all testcases.
> > Just explicitly exclude those with -x gem_concurrent. The problem is that
> > hw/kernel feature checks aren't properlty encapsulated in the right
> > igt_fixture or igt_subtest blocks, so it falls over. Specifically the
> > access_mode->require() test is only protetected by
> > igt_only_list_subtests(), which is the wrong way to do it.
> > 
> > Adding Chris.
> 
> Ok, fixed pushed now using the just added igt_subtest_group blocks. Please
> scream when anything else falls apart.

Ok, just realized that Chris' combinatorial stress-test crusade reached a
few 100k tests now ;-) So just run with -x gem_ to get rid of all the i915
gem stuff. If we get more along the lines of vc4, we probably need to give
them all a i915_ prefix. Same for the i915 ioctl wrappers probably.
-Daniel

Patch

diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c
index f2a4c0f..489e202 100644
--- a/drivers/gpu/drm/radeon/cik.c
+++ b/drivers/gpu/drm/radeon/cik.c
@@ -8496,6 +8496,142 @@  restart_ih:
 /*
  * startup/shutdown callbacks
  */
+
+static void cik_uvd_vce_init(struct radeon_device *rdev)
+{
+	struct radeon_ring *ring;
+	int r;
+
+	if (!rdev->has_uvd)
+		return;
+
+	r = radeon_uvd_init(rdev);
+	if (r)
+		goto error_uvd;
+	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
+	ring->ring_obj = NULL;
+	r600_ring_init(rdev, ring, 4096);
+
+	r = radeon_vce_init(rdev);
+	if (r)
+		goto error_vce;
+	ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
+	ring->ring_obj = NULL;
+	r600_ring_init(rdev, ring, 4096);
+	ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
+	ring->ring_obj = NULL;
+	r600_ring_init(rdev, ring, 4096);
+
+	return;
+
+error_vce:
+	radeon_uvd_fini(rdev);
+error_uvd:
+	dev_err(rdev->dev, "UVD/VCE init error (%d).\n", r);
+	/* On error just disable everything. */
+	rdev->has_uvd = 0;
+}
+
+static void cik_uvd_vce_startup(struct radeon_device *rdev)
+{
+	int r;
+
+	if (!rdev->has_uvd)
+		return;
+
+	r = uvd_v4_2_resume(rdev);
+	if (r)
+		goto error;
+	r = radeon_fence_driver_start_ring(rdev, R600_RING_TYPE_UVD_INDEX);
+	if (r)
+		goto error_uvd;
+
+	r = radeon_vce_resume(rdev);
+	if (r)
+		goto error_uvd;
+	r = vce_v2_0_resume(rdev);
+	if (r)
+		goto error_vce;
+	r = radeon_fence_driver_start_ring(rdev, TN_RING_TYPE_VCE1_INDEX);
+	if (r)
+		goto error_vce;
+	r = radeon_fence_driver_start_ring(rdev, TN_RING_TYPE_VCE2_INDEX);
+	if (r)
+		goto error_vce;
+
+	return;
+
+error_vce:
+	radeon_vce_suspend(rdev);
+error_uvd:
+	radeon_uvd_suspend(rdev);
+error:
+	dev_err(rdev->dev, "UVD/VCE startup error (%d).\n", r);
+	/* On error just disable everything. */
+	radeon_vce_fini(rdev);
+	radeon_uvd_fini(rdev);
+	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
+	rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
+	rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;
+	rdev->has_uvd = 0;
+}
+
+static void cik_uvd_vce_resume(struct radeon_device *rdev)
+{
+	struct radeon_ring *ring;
+	int r;
+
+	if (!rdev->has_uvd)
+		return;
+
+	/* On uvd/vce error we disable uvd/vce so we should have bail above. */
+	BUG_ON(!rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size);
+	BUG_ON(!rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size);
+	BUG_ON(!rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size);
+
+	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
+	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, RADEON_CP_PACKET2);
+	if (r)
+		goto error;
+	r = uvd_v1_0_init(rdev);
+	if (r)
+		goto error_uvd;
+
+	ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
+	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, VCE_CMD_NO_OP);
+	if (r)
+		goto error_vce1;
+	ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
+	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, VCE_CMD_NO_OP);
+	if (r)
+		goto error_vce2;
+	r = vce_v1_0_init(rdev);
+	if (r)
+		goto error_vce;
+
+	return;
+
+error_vce:
+	radeon_ring_fini(rdev, &rdev->ring[TN_RING_TYPE_VCE2_INDEX]);
+error_vce2:
+	radeon_ring_fini(rdev, &rdev->ring[TN_RING_TYPE_VCE1_INDEX]);
+error_vce1:
+	uvd_v1_0_fini(rdev);
+error_uvd:
+	radeon_ring_fini(rdev, &rdev->ring[R600_RING_TYPE_UVD_INDEX]);
+error:
+	dev_err(rdev->dev, "UVD/VCE resume error (%d).\n", r);
+	/* On error just disable everything. */
+	radeon_uvd_suspend(rdev);
+	radeon_vce_suspend(rdev);
+	radeon_uvd_fini(rdev);
+	radeon_vce_fini(rdev);
+	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
+	rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
+	rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;
+	rdev->has_uvd = 0;
+}
+
 /**
  * cik_startup - program the asic to a functional state
  *
@@ -8598,34 +8734,7 @@  static int cik_startup(struct radeon_device *rdev)
 		return r;
 	}
 
-	r = radeon_uvd_resume(rdev);
-	if (!r) {
-		r = uvd_v4_2_resume(rdev);
-		if (!r) {
-			r = radeon_fence_driver_start_ring(rdev,
-							   R600_RING_TYPE_UVD_INDEX);
-			if (r)
-				dev_err(rdev->dev, "UVD fences init error (%d).\n", r);
-		}
-	}
-	if (r)
-		rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
-
-	r = radeon_vce_resume(rdev);
-	if (!r) {
-		r = vce_v2_0_resume(rdev);
-		if (!r)
-			r = radeon_fence_driver_start_ring(rdev,
-							   TN_RING_TYPE_VCE1_INDEX);
-		if (!r)
-			r = radeon_fence_driver_start_ring(rdev,
-							   TN_RING_TYPE_VCE2_INDEX);
-	}
-	if (r) {
-		dev_err(rdev->dev, "VCE init error (%d).\n", r);
-		rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
-		rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;
-	}
+	cik_uvd_vce_startup(rdev);
 
 	/* Enable IRQ */
 	if (!rdev->irq.installed) {
@@ -8701,32 +8810,7 @@  static int cik_startup(struct radeon_device *rdev)
 	if (r)
 		return r;
 
-	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
-	if (ring->ring_size) {
-		r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
-				     RADEON_CP_PACKET2);
-		if (!r)
-			r = uvd_v1_0_init(rdev);
-		if (r)
-			DRM_ERROR("radeon: failed initializing UVD (%d).\n", r);
-	}
-
-	r = -ENOENT;
-
-	ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
-	if (ring->ring_size)
-		r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
-				     VCE_CMD_NO_OP);
-
-	ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
-	if (ring->ring_size)
-		r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
-				     VCE_CMD_NO_OP);
-
-	if (!r)
-		r = vce_v1_0_init(rdev);
-	else if (r != -ENOENT)
-		DRM_ERROR("radeon: failed initializing VCE (%d).\n", r);
+	cik_uvd_vce_resume(rdev);
 
 	r = radeon_ib_pool_init(rdev);
 	if (r) {
@@ -8802,9 +8886,11 @@  int cik_suspend(struct radeon_device *rdev)
 	radeon_vm_manager_fini(rdev);
 	cik_cp_enable(rdev, false);
 	cik_sdma_enable(rdev, false);
-	uvd_v1_0_fini(rdev);
-	radeon_uvd_suspend(rdev);
-	radeon_vce_suspend(rdev);
+	if (rdev->has_uvd) {
+		uvd_v1_0_fini(rdev);
+		radeon_uvd_suspend(rdev);
+		radeon_vce_suspend(rdev);
+	}
 	cik_fini_pg(rdev);
 	cik_fini_cg(rdev);
 	cik_irq_suspend(rdev);
@@ -8930,23 +9016,7 @@  int cik_init(struct radeon_device *rdev)
 	ring->ring_obj = NULL;
 	r600_ring_init(rdev, ring, 256 * 1024);
 
-	r = radeon_uvd_init(rdev);
-	if (!r) {
-		ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
-		ring->ring_obj = NULL;
-		r600_ring_init(rdev, ring, 4096);
-	}
-
-	r = radeon_vce_init(rdev);
-	if (!r) {
-		ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
-		ring->ring_obj = NULL;
-		r600_ring_init(rdev, ring, 4096);
-
-		ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
-		ring->ring_obj = NULL;
-		r600_ring_init(rdev, ring, 4096);
-	}
+	cik_uvd_vce_init(rdev);
 
 	rdev->ih.ring_obj = NULL;
 	r600_ih_ring_init(rdev, 64 * 1024);
@@ -9007,9 +9077,11 @@  void cik_fini(struct radeon_device *rdev)
 	radeon_vm_manager_fini(rdev);
 	radeon_ib_pool_fini(rdev);
 	radeon_irq_kms_fini(rdev);
-	uvd_v1_0_fini(rdev);
-	radeon_uvd_fini(rdev);
-	radeon_vce_fini(rdev);
+	if (rdev->has_uvd) {
+		uvd_v1_0_fini(rdev);
+		radeon_uvd_fini(rdev);
+		radeon_vce_fini(rdev);
+	}
 	cik_pcie_gart_fini(rdev);
 	r600_vram_scratch_fini(rdev);
 	radeon_gem_fini(rdev);
diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
index 76c4bdf..22854de 100644
--- a/drivers/gpu/drm/radeon/evergreen.c
+++ b/drivers/gpu/drm/radeon/evergreen.c
@@ -5363,6 +5363,87 @@  restart_ih:
 	return IRQ_HANDLED;
 }
 
+static void evergreen_uvd_init(struct radeon_device *rdev)
+{
+	struct radeon_ring *ring;
+	int r;
+
+	if (!rdev->has_uvd)
+		return;
+
+	r = radeon_uvd_init(rdev);
+	if (r)
+		goto error_uvd;
+	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
+	ring->ring_obj = NULL;
+	r600_ring_init(rdev, ring, 4096);
+
+	return;
+
+error_uvd:
+	dev_err(rdev->dev, "UVD init error (%d).\n", r);
+	/* On error just disable everything. */
+	rdev->has_uvd = 0;
+}
+
+static void evergreen_uvd_startup(struct radeon_device *rdev)
+{
+	int r;
+
+	if (!rdev->has_uvd)
+		return;
+
+	r = uvd_v2_2_resume(rdev);
+	if (r)
+		goto error;
+	r = radeon_fence_driver_start_ring(rdev, R600_RING_TYPE_UVD_INDEX);
+	if (r)
+		goto error_uvd;
+
+	return;
+
+error_uvd:
+	radeon_uvd_suspend(rdev);
+error:
+	dev_err(rdev->dev, "UVD startup error (%d).\n", r);
+	/* On error just disable everything. */
+	radeon_uvd_fini(rdev);
+	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
+	rdev->has_uvd = 0;
+}
+
+static void evergreen_uvd_resume(struct radeon_device *rdev)
+{
+	struct radeon_ring *ring;
+	int r;
+
+	if (!rdev->has_uvd)
+		return;
+
+	/* On uvd error we disable uvd so we should have bail above. */
+	BUG_ON(!rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size);
+
+	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
+	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, RADEON_CP_PACKET2);
+	if (r)
+		goto error;
+	r = uvd_v1_0_init(rdev);
+	if (r)
+		goto error_uvd;
+
+	return;
+
+error_uvd:
+	radeon_ring_fini(rdev, &rdev->ring[R600_RING_TYPE_UVD_INDEX]);
+error:
+	dev_err(rdev->dev, "UVD resume error (%d).\n", r);
+	/* On error just disable everything. */
+	radeon_uvd_suspend(rdev);
+	radeon_uvd_fini(rdev);
+	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
+	rdev->has_uvd = 0;
+}
+
 static int evergreen_startup(struct radeon_device *rdev)
 {
 	struct radeon_ring *ring;
@@ -5427,16 +5508,7 @@  static int evergreen_startup(struct radeon_device *rdev)
 		return r;
 	}
 
-	r = uvd_v2_2_resume(rdev);
-	if (!r) {
-		r = radeon_fence_driver_start_ring(rdev,
-						   R600_RING_TYPE_UVD_INDEX);
-		if (r)
-			dev_err(rdev->dev, "UVD fences init error (%d).\n", r);
-	}
-
-	if (r)
-		rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
+	evergreen_uvd_startup(rdev);
 
 	/* Enable IRQ */
 	if (!rdev->irq.installed) {
@@ -5475,16 +5547,7 @@  static int evergreen_startup(struct radeon_device *rdev)
 	if (r)
 		return r;
 
-	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
-	if (ring->ring_size) {
-		r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
-				     RADEON_CP_PACKET2);
-		if (!r)
-			r = uvd_v1_0_init(rdev);
-
-		if (r)
-			DRM_ERROR("radeon: error initializing UVD (%d).\n", r);
-	}
+	evergreen_uvd_resume(rdev);
 
 	r = radeon_ib_pool_init(rdev);
 	if (r) {
@@ -5539,8 +5602,10 @@  int evergreen_suspend(struct radeon_device *rdev)
 {
 	radeon_pm_suspend(rdev);
 	radeon_audio_fini(rdev);
-	uvd_v1_0_fini(rdev);
-	radeon_uvd_suspend(rdev);
+	if (rdev->has_uvd) {
+		uvd_v1_0_fini(rdev);
+		radeon_uvd_suspend(rdev);
+	}
 	r700_cp_stop(rdev);
 	r600_dma_stop(rdev);
 	evergreen_irq_suspend(rdev);
@@ -5641,12 +5706,7 @@  int evergreen_init(struct radeon_device *rdev)
 	rdev->ring[R600_RING_TYPE_DMA_INDEX].ring_obj = NULL;
 	r600_ring_init(rdev, &rdev->ring[R600_RING_TYPE_DMA_INDEX], 64 * 1024);
 
-	r = radeon_uvd_init(rdev);
-	if (!r) {
-		rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_obj = NULL;
-		r600_ring_init(rdev, &rdev->ring[R600_RING_TYPE_UVD_INDEX],
-			       4096);
-	}
+	evergreen_uvd_init(rdev);
 
 	rdev->ih.ring_obj = NULL;
 	r600_ih_ring_init(rdev, 64 * 1024);
@@ -5697,8 +5757,10 @@  void evergreen_fini(struct radeon_device *rdev)
 	radeon_wb_fini(rdev);
 	radeon_ib_pool_fini(rdev);
 	radeon_irq_kms_fini(rdev);
-	uvd_v1_0_fini(rdev);
-	radeon_uvd_fini(rdev);
+	if (rdev->has_uvd) {
+		uvd_v1_0_fini(rdev);
+		radeon_uvd_fini(rdev);
+	}
 	evergreen_pcie_gart_fini(rdev);
 	r600_vram_scratch_fini(rdev);
 	radeon_gem_fini(rdev);
diff --git a/drivers/gpu/drm/radeon/ni.c b/drivers/gpu/drm/radeon/ni.c
index b88d63c9..74f7316 100644
--- a/drivers/gpu/drm/radeon/ni.c
+++ b/drivers/gpu/drm/radeon/ni.c
@@ -2002,6 +2002,154 @@  bool cayman_gfx_is_lockup(struct radeon_device *rdev, struct radeon_ring *ring)
 	return radeon_ring_test_lockup(rdev, ring);
 }
 
+static void cayman_uvd_vce_init(struct radeon_device *rdev)
+{
+	struct radeon_ring *ring;
+	int r;
+
+	if (!rdev->has_uvd)
+		return;
+
+	r = radeon_uvd_init(rdev);
+	if (r)
+		goto error_uvd;
+	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
+	ring->ring_obj = NULL;
+	r600_ring_init(rdev, ring, 4096);
+
+	if (rdev->family != CHIP_ARUBA)
+		return;
+
+	r = radeon_vce_init(rdev);
+	if (r)
+		goto error_vce;
+	ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
+	ring->ring_obj = NULL;
+	r600_ring_init(rdev, ring, 4096);
+	ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
+	ring->ring_obj = NULL;
+	r600_ring_init(rdev, ring, 4096);
+
+	return;
+
+error_vce:
+	radeon_uvd_fini(rdev);
+error_uvd:
+	dev_err(rdev->dev, "UVD/VCE init error (%d).\n", r);
+	/* On error just disable everything. */
+	rdev->has_uvd = 0;
+}
+
+static void cayman_uvd_vce_startup(struct radeon_device *rdev)
+{
+	int r;
+
+	if (!rdev->has_uvd)
+		return;
+
+	r = uvd_v2_2_resume(rdev);
+	if (r)
+		goto error;
+	r = radeon_fence_driver_start_ring(rdev, R600_RING_TYPE_UVD_INDEX);
+	if (r)
+		goto error_uvd;
+
+	/* VCE only CHIP_ARUBA */
+	if (rdev->family != CHIP_ARUBA)
+		return;
+	r = radeon_vce_resume(rdev);
+	if (r)
+		goto error_uvd;
+	r = vce_v1_0_resume(rdev);
+	if (r)
+		goto error_vce;
+	r = radeon_fence_driver_start_ring(rdev, TN_RING_TYPE_VCE1_INDEX);
+	if (r)
+		goto error_vce;
+	r = radeon_fence_driver_start_ring(rdev, TN_RING_TYPE_VCE2_INDEX);
+	if (r)
+		goto error_vce;
+
+	return;
+
+error_vce:
+	radeon_vce_suspend(rdev);
+error_uvd:
+	radeon_uvd_suspend(rdev);
+error:
+	dev_err(rdev->dev, "UVD/VCE startup error (%d).\n", r);
+	/* On error just disable everything. */
+	radeon_uvd_fini(rdev);
+	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
+	if (rdev->family == CHIP_ARUBA) {
+		radeon_vce_fini(rdev);
+		rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
+		rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;
+	}
+	rdev->has_uvd = 0;
+}
+
+static void cayman_uvd_vce_resume(struct radeon_device *rdev)
+{
+	struct radeon_ring *ring;
+	int r;
+
+	if (!rdev->has_uvd)
+		return;
+
+	/* On uvd/vce error we disable uvd/vce so we should have bail above. */
+	BUG_ON(!rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size);
+
+	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
+	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, RADEON_CP_PACKET2);
+	if (r)
+		goto error;
+	r = uvd_v1_0_init(rdev);
+	if (r)
+		goto error_uvd;
+
+	/* VCE only CHIP_ARUBA */
+	if (rdev->family != CHIP_ARUBA)
+		return;
+	BUG_ON(!rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size);
+	BUG_ON(!rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size);
+	ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
+	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, 0x0);
+	if (r)
+		goto error_vce1;
+	ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
+	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, 0x0);
+	if (r)
+		goto error_vce2;
+	r = vce_v1_0_init(rdev);
+	if (r)
+		goto error_vce;
+
+	return;
+
+error_vce:
+	radeon_ring_fini(rdev, &rdev->ring[TN_RING_TYPE_VCE2_INDEX]);
+error_vce2:
+	radeon_ring_fini(rdev, &rdev->ring[TN_RING_TYPE_VCE1_INDEX]);
+error_vce1:
+	uvd_v1_0_fini(rdev);
+	radeon_vce_suspend(rdev);
+error_uvd:
+	radeon_ring_fini(rdev, &rdev->ring[R600_RING_TYPE_UVD_INDEX]);
+error:
+	dev_err(rdev->dev, "UVD/VCE resume error (%d).\n", r);
+	/* On error just disable everything. */
+	radeon_uvd_suspend(rdev);
+	radeon_uvd_fini(rdev);
+	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
+	if (rdev->family == CHIP_ARUBA) {
+		radeon_vce_fini(rdev);
+		rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
+		rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;
+	}
+	rdev->has_uvd = 0;
+}
+
 static int cayman_startup(struct radeon_device *rdev)
 {
 	struct radeon_ring *ring = &rdev->ring[RADEON_RING_TYPE_GFX_INDEX];
@@ -2056,34 +2204,7 @@  static int cayman_startup(struct radeon_device *rdev)
 		return r;
 	}
 
-	r = uvd_v2_2_resume(rdev);
-	if (!r) {
-		r = radeon_fence_driver_start_ring(rdev,
-						   R600_RING_TYPE_UVD_INDEX);
-		if (r)
-			dev_err(rdev->dev, "UVD fences init error (%d).\n", r);
-	}
-	if (r)
-		rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
-
-	if (rdev->family == CHIP_ARUBA) {
-		r = radeon_vce_resume(rdev);
-		if (!r)
-			r = vce_v1_0_resume(rdev);
-
-		if (!r)
-			r = radeon_fence_driver_start_ring(rdev,
-							   TN_RING_TYPE_VCE1_INDEX);
-		if (!r)
-			r = radeon_fence_driver_start_ring(rdev,
-							   TN_RING_TYPE_VCE2_INDEX);
-
-		if (r) {
-			dev_err(rdev->dev, "VCE init error (%d).\n", r);
-			rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
-			rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;
-		}
-	}
+	cayman_uvd_vce_startup(rdev);
 
 	r = radeon_fence_driver_start_ring(rdev, CAYMAN_RING_TYPE_CP1_INDEX);
 	if (r) {
@@ -2152,30 +2273,7 @@  static int cayman_startup(struct radeon_device *rdev)
 	if (r)
 		return r;
 
-	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
-	if (ring->ring_size) {
-		r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
-				     RADEON_CP_PACKET2);
-		if (!r)
-			r = uvd_v1_0_init(rdev);
-		if (r)
-			DRM_ERROR("radeon: failed initializing UVD (%d).\n", r);
-	}
-
-	if (rdev->family == CHIP_ARUBA) {
-		ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
-		if (ring->ring_size)
-			r = radeon_ring_init(rdev, ring, ring->ring_size, 0, 0x0);
-
-		ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
-		if (ring->ring_size)
-			r = radeon_ring_init(rdev, ring, ring->ring_size, 0, 0x0);
-
-		if (!r)
-			r = vce_v1_0_init(rdev);
-		if (r)
-			DRM_ERROR("radeon: failed initializing VCE (%d).\n", r);
-	}
+	cayman_uvd_vce_resume(rdev);
 
 	r = radeon_ib_pool_init(rdev);
 	if (r) {
@@ -2230,8 +2328,12 @@  int cayman_suspend(struct radeon_device *rdev)
 	radeon_vm_manager_fini(rdev);
 	cayman_cp_enable(rdev, false);
 	cayman_dma_stop(rdev);
-	uvd_v1_0_fini(rdev);
-	radeon_uvd_suspend(rdev);
+	if (rdev->has_uvd) {
+		uvd_v1_0_fini(rdev);
+		radeon_uvd_suspend(rdev);
+		if (rdev->family == CHIP_ARUBA)
+			radeon_vce_suspend(rdev);
+	}
 	evergreen_irq_suspend(rdev);
 	radeon_wb_disable(rdev);
 	cayman_pcie_gart_disable(rdev);
@@ -2325,25 +2427,7 @@  int cayman_init(struct radeon_device *rdev)
 	ring->ring_obj = NULL;
 	r600_ring_init(rdev, ring, 64 * 1024);
 
-	r = radeon_uvd_init(rdev);
-	if (!r) {
-		ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
-		ring->ring_obj = NULL;
-		r600_ring_init(rdev, ring, 4096);
-	}
-
-	if (rdev->family == CHIP_ARUBA) {
-		r = radeon_vce_init(rdev);
-		if (!r) {
-			ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
-			ring->ring_obj = NULL;
-			r600_ring_init(rdev, ring, 4096);
-
-			ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
-			ring->ring_obj = NULL;
-			r600_ring_init(rdev, ring, 4096);
-		}
-	}
+	cayman_uvd_vce_init(rdev);
 
 	rdev->ih.ring_obj = NULL;
 	r600_ih_ring_init(rdev, 64 * 1024);
@@ -2396,10 +2480,12 @@  void cayman_fini(struct radeon_device *rdev)
 	radeon_vm_manager_fini(rdev);
 	radeon_ib_pool_fini(rdev);
 	radeon_irq_kms_fini(rdev);
-	uvd_v1_0_fini(rdev);
-	radeon_uvd_fini(rdev);
-	if (rdev->family == CHIP_ARUBA)
-		radeon_vce_fini(rdev);
+	if (rdev->has_uvd) {
+		uvd_v1_0_fini(rdev);
+		radeon_uvd_fini(rdev);
+		if (rdev->family == CHIP_ARUBA)
+			radeon_vce_fini(rdev);
+	}
 	cayman_pcie_gart_fini(rdev);
 	r600_vram_scratch_fini(rdev);
 	radeon_gem_fini(rdev);
diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
index f86ab69..9d95bee 100644
--- a/drivers/gpu/drm/radeon/r600.c
+++ b/drivers/gpu/drm/radeon/r600.c
@@ -3035,6 +3035,87 @@  void r600_clear_surface_reg(struct radeon_device *rdev, int reg)
 	/* FIXME: implement */
 }
 
+static void r600_uvd_init(struct radeon_device *rdev)
+{
+	struct radeon_ring *ring;
+	int r;
+
+	if (!rdev->has_uvd)
+		return;
+
+	r = radeon_uvd_init(rdev);
+	if (r)
+		goto error_uvd;
+	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
+	ring->ring_obj = NULL;
+	r600_ring_init(rdev, ring, 4096);
+
+	return;
+
+error_uvd:
+	dev_err(rdev->dev, "UVD init error (%d).\n", r);
+	/* On error just disable everything. */
+	rdev->has_uvd = 0;
+}
+
+static void r600_uvd_startup(struct radeon_device *rdev)
+{
+	int r;
+
+	if (!rdev->has_uvd)
+		return;
+
+	r = uvd_v1_0_resume(rdev);
+	if (r)
+		goto error;
+	r = radeon_fence_driver_start_ring(rdev, R600_RING_TYPE_UVD_INDEX);
+	if (r)
+		goto error_uvd;
+
+	return;
+
+error_uvd:
+	radeon_uvd_suspend(rdev);
+error:
+	dev_err(rdev->dev, "UVD startup error (%d).\n", r);
+	/* On error just disable everything. */
+	radeon_uvd_fini(rdev);
+	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
+	rdev->has_uvd = 0;
+}
+
+static void r600_uvd_resume(struct radeon_device *rdev)
+{
+	struct radeon_ring *ring;
+	int r;
+
+	if (!rdev->has_uvd)
+		return;
+
+	/* On uvd error we disable uvd so we should have bail above. */
+	BUG_ON(!rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size);
+
+	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
+	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, RADEON_CP_PACKET2);
+	if (r)
+		goto error;
+	r = uvd_v1_0_init(rdev);
+	if (r)
+		goto error_uvd;
+
+	return;
+
+error_uvd:
+	radeon_ring_fini(rdev, &rdev->ring[R600_RING_TYPE_UVD_INDEX]);
+error:
+	dev_err(rdev->dev, "UVD resume error (%d).\n", r);
+	/* On error just disable everything. */
+	radeon_uvd_suspend(rdev);
+	radeon_uvd_fini(rdev);
+	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
+	rdev->has_uvd = 0;
+}
+
 static int r600_startup(struct radeon_device *rdev)
 {
 	struct radeon_ring *ring;
@@ -3070,17 +3151,7 @@  static int r600_startup(struct radeon_device *rdev)
 		return r;
 	}
 
-	if (rdev->has_uvd) {
-		r = uvd_v1_0_resume(rdev);
-		if (!r) {
-			r = radeon_fence_driver_start_ring(rdev, R600_RING_TYPE_UVD_INDEX);
-			if (r) {
-				dev_err(rdev->dev, "failed initializing UVD fences (%d).\n", r);
-			}
-		}
-		if (r)
-			rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
-	}
+	r600_uvd_startup(rdev);
 
 	/* Enable IRQ */
 	if (!rdev->irq.installed) {
@@ -3110,17 +3181,7 @@  static int r600_startup(struct radeon_device *rdev)
 	if (r)
 		return r;
 
-	if (rdev->has_uvd) {
-		ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
-		if (ring->ring_size) {
-			r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
-					     RADEON_CP_PACKET2);
-			if (!r)
-				r = uvd_v1_0_init(rdev);
-			if (r)
-				DRM_ERROR("radeon: failed initializing UVD (%d).\n", r);
-		}
-	}
+	r600_uvd_resume(rdev);
 
 	r = radeon_ib_pool_init(rdev);
 	if (r) {
@@ -3264,13 +3325,7 @@  int r600_init(struct radeon_device *rdev)
 	rdev->ring[RADEON_RING_TYPE_GFX_INDEX].ring_obj = NULL;
 	r600_ring_init(rdev, &rdev->ring[RADEON_RING_TYPE_GFX_INDEX], 1024 * 1024);
 
-	if (rdev->has_uvd) {
-		r = radeon_uvd_init(rdev);
-		if (!r) {
-			rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_obj = NULL;
-			r600_ring_init(rdev, &rdev->ring[R600_RING_TYPE_UVD_INDEX], 4096);
-		}
-	}
+	r600_uvd_init(rdev);
 
 	rdev->ih.ring_obj = NULL;
 	r600_ih_ring_init(rdev, 64 * 1024);
diff --git a/drivers/gpu/drm/radeon/rv770.c b/drivers/gpu/drm/radeon/rv770.c
index 01ee96a..cf6e2ee 100644
--- a/drivers/gpu/drm/radeon/rv770.c
+++ b/drivers/gpu/drm/radeon/rv770.c
@@ -1681,6 +1681,87 @@  static int rv770_mc_init(struct radeon_device *rdev)
 	return 0;
 }
 
+static void rv770_uvd_init(struct radeon_device *rdev)
+{
+	struct radeon_ring *ring;
+	int r;
+
+	if (!rdev->has_uvd)
+		return;
+
+	r = radeon_uvd_init(rdev);
+	if (r)
+		goto error_uvd;
+	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
+	ring->ring_obj = NULL;
+	r600_ring_init(rdev, ring, 4096);
+
+	return;
+
+error_uvd:
+	dev_err(rdev->dev, "UVD init error (%d).\n", r);
+	/* On error just disable everything. */
+	rdev->has_uvd = 0;
+}
+
+static void rv770_uvd_startup(struct radeon_device *rdev)
+{
+	int r;
+
+	if (!rdev->has_uvd)
+		return;
+
+	r = uvd_v2_2_resume(rdev);
+	if (r)
+		goto error;
+	r = radeon_fence_driver_start_ring(rdev, R600_RING_TYPE_UVD_INDEX);
+	if (r)
+		goto error_uvd;
+
+	return;
+
+error_uvd:
+	radeon_uvd_suspend(rdev);
+error:
+	dev_err(rdev->dev, "UVD startup error (%d).\n", r);
+	/* On error just disable everything. */
+	radeon_uvd_fini(rdev);
+	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
+	rdev->has_uvd = 0;
+}
+
+static void rv770_uvd_resume(struct radeon_device *rdev)
+{
+	struct radeon_ring *ring;
+	int r;
+
+	if (!rdev->has_uvd)
+		return;
+
+	/* On uvd error we disable uvd so we should have bail above. */
+	BUG_ON(!rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size);
+
+	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
+	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, RADEON_CP_PACKET2);
+	if (r)
+		goto error;
+	r = uvd_v1_0_init(rdev);
+	if (r)
+		goto error_uvd;
+
+	return;
+
+error_uvd:
+	radeon_ring_fini(rdev, &rdev->ring[R600_RING_TYPE_UVD_INDEX]);
+error:
+	dev_err(rdev->dev, "UVD resume error (%d).\n", r);
+	/* On error just disable everything. */
+	radeon_uvd_suspend(rdev);
+	radeon_uvd_fini(rdev);
+	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
+	rdev->has_uvd = 0;
+}
+
 static int rv770_startup(struct radeon_device *rdev)
 {
 	struct radeon_ring *ring;
@@ -1723,16 +1804,7 @@  static int rv770_startup(struct radeon_device *rdev)
 		return r;
 	}
 
-	r = uvd_v2_2_resume(rdev);
-	if (!r) {
-		r = radeon_fence_driver_start_ring(rdev,
-						   R600_RING_TYPE_UVD_INDEX);
-		if (r)
-			dev_err(rdev->dev, "UVD fences init error (%d).\n", r);
-	}
-
-	if (r)
-		rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
+	rv770_uvd_startup(rdev);
 
 	/* Enable IRQ */
 	if (!rdev->irq.installed) {
@@ -1772,16 +1844,7 @@  static int rv770_startup(struct radeon_device *rdev)
 	if (r)
 		return r;
 
-	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
-	if (ring->ring_size) {
-		r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
-				     RADEON_CP_PACKET2);
-		if (!r)
-			r = uvd_v1_0_init(rdev);
-
-		if (r)
-			DRM_ERROR("radeon: failed initializing UVD (%d).\n", r);
-	}
+	rv770_uvd_resume(rdev);
 
 	r = radeon_ib_pool_init(rdev);
 	if (r) {
@@ -1831,8 +1894,10 @@  int rv770_suspend(struct radeon_device *rdev)
 {
 	radeon_pm_suspend(rdev);
 	radeon_audio_fini(rdev);
-	uvd_v1_0_fini(rdev);
-	radeon_uvd_suspend(rdev);
+	if (rdev->has_uvd) {
+		uvd_v1_0_fini(rdev);
+		radeon_uvd_suspend(rdev);
+	}
 	r700_cp_stop(rdev);
 	r600_dma_stop(rdev);
 	r600_irq_suspend(rdev);
@@ -1917,12 +1982,7 @@  int rv770_init(struct radeon_device *rdev)
 	rdev->ring[R600_RING_TYPE_DMA_INDEX].ring_obj = NULL;
 	r600_ring_init(rdev, &rdev->ring[R600_RING_TYPE_DMA_INDEX], 64 * 1024);
 
-	r = radeon_uvd_init(rdev);
-	if (!r) {
-		rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_obj = NULL;
-		r600_ring_init(rdev, &rdev->ring[R600_RING_TYPE_UVD_INDEX],
-			       4096);
-	}
+	rv770_uvd_init(rdev);
 
 	rdev->ih.ring_obj = NULL;
 	r600_ih_ring_init(rdev, 64 * 1024);
@@ -1957,8 +2017,10 @@  void rv770_fini(struct radeon_device *rdev)
 	radeon_wb_fini(rdev);
 	radeon_ib_pool_fini(rdev);
 	radeon_irq_kms_fini(rdev);
-	uvd_v1_0_fini(rdev);
-	radeon_uvd_fini(rdev);
+	if (rdev->has_uvd) {
+		uvd_v1_0_fini(rdev);
+		radeon_uvd_fini(rdev);
+	}
 	rv770_pcie_gart_fini(rdev);
 	r600_vram_scratch_fini(rdev);
 	radeon_gem_fini(rdev);
diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c
index e894be2..7d8d5f5 100644
--- a/drivers/gpu/drm/radeon/si.c
+++ b/drivers/gpu/drm/radeon/si.c
@@ -6868,6 +6868,142 @@  restart_ih:
 /*
  * startup/shutdown callbacks
  */
+
+static void si_uvd_vce_init(struct radeon_device *rdev)
+{
+	struct radeon_ring *ring;
+	int r;
+
+	if (!rdev->has_uvd)
+		return;
+
+	r = radeon_uvd_init(rdev);
+	if (r)
+		goto error_uvd;
+	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
+	ring->ring_obj = NULL;
+	r600_ring_init(rdev, ring, 4096);
+
+	r = radeon_vce_init(rdev);
+	if (r)
+		goto error_vce;
+	ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
+	ring->ring_obj = NULL;
+	r600_ring_init(rdev, ring, 4096);
+	ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
+	ring->ring_obj = NULL;
+	r600_ring_init(rdev, ring, 4096);
+
+	return;
+
+error_vce:
+	radeon_uvd_fini(rdev);
+error_uvd:
+	dev_err(rdev->dev, "UVD/VCE init error (%d).\n", r);
+	/* On error just disable everything. */
+	rdev->has_uvd = 0;
+}
+
+static void si_uvd_vce_startup(struct radeon_device *rdev)
+{
+	int r;
+
+	if (!rdev->has_uvd)
+		return;
+
+	r = uvd_v2_2_resume(rdev);
+	if (r)
+		goto error;
+	r = radeon_fence_driver_start_ring(rdev, R600_RING_TYPE_UVD_INDEX);
+	if (r)
+		goto error_uvd;
+
+	r = radeon_vce_resume(rdev);
+	if (r)
+		goto error_uvd;
+	r = vce_v1_0_resume(rdev);
+	if (r)
+		goto error_vce;
+	r = radeon_fence_driver_start_ring(rdev, TN_RING_TYPE_VCE1_INDEX);
+	if (r)
+		goto error_vce;
+	r = radeon_fence_driver_start_ring(rdev, TN_RING_TYPE_VCE2_INDEX);
+	if (r)
+		goto error_vce;
+
+	return;
+
+error_vce:
+	radeon_vce_suspend(rdev);
+error_uvd:
+	radeon_uvd_suspend(rdev);
+error:
+	dev_err(rdev->dev, "UVD/VCE startup error (%d).\n", r);
+	/* On error just disable everything. */
+	radeon_vce_fini(rdev);
+	radeon_uvd_fini(rdev);
+	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
+	rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
+	rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;
+	rdev->has_uvd = 0;
+}
+
+static void si_uvd_vce_resume(struct radeon_device *rdev)
+{
+	struct radeon_ring *ring;
+	int r;
+
+	if (!rdev->has_uvd)
+		return;
+
+	/* On uvd/vce error we disable uvd/vce so we should have bail above. */
+	BUG_ON(!rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size);
+	BUG_ON(!rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size);
+	BUG_ON(!rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size);
+
+	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
+	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, RADEON_CP_PACKET2);
+	if (r)
+		goto error;
+	r = uvd_v1_0_init(rdev);
+	if (r)
+		goto error_uvd;
+
+	ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
+	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, VCE_CMD_NO_OP);
+	if (r)
+		goto error_vce1;
+	ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
+	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, VCE_CMD_NO_OP);
+	if (r)
+		goto error_vce2;
+	r = vce_v1_0_init(rdev);
+	if (r)
+		goto error_vce;
+
+	return;
+
+error_vce:
+	radeon_ring_fini(rdev, &rdev->ring[TN_RING_TYPE_VCE2_INDEX]);
+error_vce2:
+	radeon_ring_fini(rdev, &rdev->ring[TN_RING_TYPE_VCE1_INDEX]);
+error_vce1:
+	uvd_v1_0_fini(rdev);
+error_uvd:
+	radeon_ring_fini(rdev, &rdev->ring[R600_RING_TYPE_UVD_INDEX]);
+error:
+	dev_err(rdev->dev, "UVD/VCE resume error (%d).\n", r);
+	/* On error just disable everything. */
+	radeon_uvd_suspend(rdev);
+	radeon_vce_suspend(rdev);
+	radeon_uvd_fini(rdev);
+	radeon_vce_fini(rdev);
+	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
+	rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
+	rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;
+	rdev->has_uvd = 0;
+}
+
 static int si_startup(struct radeon_device *rdev)
 {
 	struct radeon_ring *ring;
@@ -6946,33 +7082,7 @@  static int si_startup(struct radeon_device *rdev)
 		return r;
 	}
 
-	if (rdev->has_uvd) {
-		r = uvd_v2_2_resume(rdev);
-		if (!r) {
-			r = radeon_fence_driver_start_ring(rdev,
-							   R600_RING_TYPE_UVD_INDEX);
-			if (r)
-				dev_err(rdev->dev, "UVD fences init error (%d).\n", r);
-		}
-		if (r)
-			rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
-	}
-
-	r = radeon_vce_resume(rdev);
-	if (!r) {
-		r = vce_v1_0_resume(rdev);
-		if (!r)
-			r = radeon_fence_driver_start_ring(rdev,
-							   TN_RING_TYPE_VCE1_INDEX);
-		if (!r)
-			r = radeon_fence_driver_start_ring(rdev,
-							   TN_RING_TYPE_VCE2_INDEX);
-	}
-	if (r) {
-		dev_err(rdev->dev, "VCE init error (%d).\n", r);
-		rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
-		rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;
-	}
+	si_uvd_vce_startup(rdev);
 
 	/* Enable IRQ */
 	if (!rdev->irq.installed) {
@@ -7030,34 +7140,7 @@  static int si_startup(struct radeon_device *rdev)
 	if (r)
 		return r;
 
-	if (rdev->has_uvd) {
-		ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
-		if (ring->ring_size) {
-			r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
-					     RADEON_CP_PACKET2);
-			if (!r)
-				r = uvd_v1_0_init(rdev);
-			if (r)
-				DRM_ERROR("radeon: failed initializing UVD (%d).\n", r);
-		}
-	}
-
-	r = -ENOENT;
-
-	ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
-	if (ring->ring_size)
-		r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
-				     VCE_CMD_NO_OP);
-
-	ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
-	if (ring->ring_size)
-		r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
-				     VCE_CMD_NO_OP);
-
-	if (!r)
-		r = vce_v1_0_init(rdev);
-	else if (r != -ENOENT)
-		DRM_ERROR("radeon: failed initializing VCE (%d).\n", r);
+	si_uvd_vce_resume(rdev);
 
 	r = radeon_ib_pool_init(rdev);
 	if (r) {
@@ -7216,25 +7299,7 @@  int si_init(struct radeon_device *rdev)
 	ring->ring_obj = NULL;
 	r600_ring_init(rdev, ring, 64 * 1024);
 
-	if (rdev->has_uvd) {
-		r = radeon_uvd_init(rdev);
-		if (!r) {
-			ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
-			ring->ring_obj = NULL;
-			r600_ring_init(rdev, ring, 4096);
-		}
-	}
-
-	r = radeon_vce_init(rdev);
-	if (!r) {
-		ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
-		ring->ring_obj = NULL;
-		r600_ring_init(rdev, ring, 4096);
-
-		ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
-		ring->ring_obj = NULL;
-		r600_ring_init(rdev, ring, 4096);
-	}
+	si_uvd_vce_init(rdev);
 
 	rdev->ih.ring_obj = NULL;
 	r600_ih_ring_init(rdev, 64 * 1024);
diff --git a/drivers/gpu/drm/radeon/uvd_v4_2.c b/drivers/gpu/drm/radeon/uvd_v4_2.c
index d04d507..d7e4807 100644
--- a/drivers/gpu/drm/radeon/uvd_v4_2.c
+++ b/drivers/gpu/drm/radeon/uvd_v4_2.c
@@ -39,6 +39,11 @@  int uvd_v4_2_resume(struct radeon_device *rdev)
 {
 	uint64_t addr;
 	uint32_t size;
+	int r;
+
+	r = radeon_uvd_resume(rdev);
+	if (r)
+		return r;
 
 	/* programm the VCPU memory controller bits 0-27 */
 	addr = rdev->uvd.gpu_addr >> 3;