diff mbox

[01/14] drm/radeon: consolidate r600 uvd initialization and startup code.

Message ID 1458316719-30104-2-git-send-email-jglisse@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jerome Glisse March 18, 2016, 3:58 p.m. UTC 
From: Jérome Glisse <jglisse@redhat.com>

This match the exact same control flow as existing code. It just
use goto instead of multiple levels of if/else. It also clarify
early initialization failures by clearing rdev->has_uvd doing so
does not change end result from hardware point of view, it only
avoids printing more error messages down the line and thus only
the original error is reported.

Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/radeon/r600.c | 99 ++++++++++++++++++++++++++++++-------------
 1 file changed, 70 insertions(+), 29 deletions(-)

Comments

Christian König March 18, 2016, 4:44 p.m. UTC | #1 
Am 18.03.2016 um 16:58 schrieb Jérôme Glisse:
> From: Jérome Glisse <jglisse@redhat.com>
>
> This match the exact same control flow as existing code. It just
> use goto instead of multiple levels of if/else. It also clarify
> early initialization failures by clearing rdev->has_uvd doing so
> does not change end result from hardware point of view, it only
> avoids printing more error messages down the line and thus only
> the original error is reported.
>
> Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Christian König <christian.koenig@amd.com>

Patches #7 and #11-#14 are Reviewed-by: Christian König 
<christian.koenig@amd.com>

Patches #1-#6 and #8-#10 are Acked-by: Christian König 
<christian.koenig@amd.com>

Thanks,
Christian.

> ---
>   drivers/gpu/drm/radeon/r600.c | 99 ++++++++++++++++++++++++++++++-------------
>   1 file changed, 70 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
> index f86ab69..24fa982 100644
> --- a/drivers/gpu/drm/radeon/r600.c
> +++ b/drivers/gpu/drm/radeon/r600.c
> @@ -3035,6 +3035,73 @@ void r600_clear_surface_reg(struct radeon_device *rdev, int reg)
>   	/* FIXME: implement */
>   }
>   
> +static void r600_uvd_init(struct radeon_device *rdev)
> +{
> +	int r;
> +
> +	if (!rdev->has_uvd)
> +		return;
> +
> +	r = radeon_uvd_init(rdev);
> +	if (r) {
> +		dev_err(rdev->dev, "failed UVD (%d) init.\n", r);
> +		/*
> +		 * At this point rdev->uvd.vcpu_bo is NULL which trickles down
> +		 * to early fails uvd_v1_0_resume() and thus nothing happens
> +		 * there. So it is pointless to try to go through that code
> +		 * hence why we disable uvd here.
> +		 */
> +		rdev->has_uvd = 0;
> +		return;
> +	}
> +	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_obj = NULL;
> +	r600_ring_init(rdev, &rdev->ring[R600_RING_TYPE_UVD_INDEX], 4096);
> +}
> +
> +static void r600_uvd_start(struct radeon_device *rdev)
> +{
> +	int r;
> +
> +	if (!rdev->has_uvd)
> +		return;
> +
> +	r = uvd_v1_0_resume(rdev);
> +	if (r) {
> +		dev_err(rdev->dev, "failed UVD resume (%d).\n", r);
> +		goto error;
> +	}
> +	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);
> +		goto error;
> +	}
> +	return;
> +
> +error:
> +	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
> +}
> +
> +static void r600_uvd_resume(struct radeon_device *rdev)
> +{
> +	struct radeon_ring *ring;
> +	int r;
> +
> +	if (!rdev->has_uvd || !rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size)
> +		return;
> +
> +	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
> +	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, RADEON_CP_PACKET2);
> +	if (r) {
> +		dev_err(rdev->dev, "failed initializing UVD ring (%d).\n", r);
> +		return;
> +	}
> +	r = uvd_v1_0_init(rdev);
> +	if (r) {
> +		dev_err(rdev->dev, "failed initializing UVD (%d).\n", r);
> +		return;
> +	}
> +}
> +
>   static int r600_startup(struct radeon_device *rdev)
>   {
>   	struct radeon_ring *ring;
> @@ -3070,17 +3137,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_start(rdev);
>   
>   	/* Enable IRQ */
>   	if (!rdev->irq.installed) {
> @@ -3110,17 +3167,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 +3311,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);
Alex Deucher March 21, 2016, 3:49 p.m. UTC | #2 
On Fri, Mar 18, 2016 at 12:44 PM, Christian König
<christian.koenig@amd.com> wrote:
> Am 18.03.2016 um 16:58 schrieb Jérôme Glisse:
>>
>> From: Jérome Glisse <jglisse@redhat.com>
>>
>> This match the exact same control flow as existing code. It just
>> use goto instead of multiple levels of if/else. It also clarify
>> early initialization failures by clearing rdev->has_uvd doing so
>> does not change end result from hardware point of view, it only
>> avoids printing more error messages down the line and thus only
>> the original error is reported.
>>
>> Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> Cc: Christian König <christian.koenig@amd.com>
>
>
> Patches #7 and #11-#14 are Reviewed-by: Christian König
> <christian.koenig@amd.com>
>
> Patches #1-#6 and #8-#10 are Acked-by: Christian König
> <christian.koenig@amd.com>
>

Applied.  thanks!

Alex

> Thanks,
> Christian.
>
>
>> ---
>>   drivers/gpu/drm/radeon/r600.c | 99
>> ++++++++++++++++++++++++++++++-------------
>>   1 file changed, 70 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
>> index f86ab69..24fa982 100644
>> --- a/drivers/gpu/drm/radeon/r600.c
>> +++ b/drivers/gpu/drm/radeon/r600.c
>> @@ -3035,6 +3035,73 @@ void r600_clear_surface_reg(struct radeon_device
>> *rdev, int reg)
>>         /* FIXME: implement */
>>   }
>>   +static void r600_uvd_init(struct radeon_device *rdev)
>> +{
>> +       int r;
>> +
>> +       if (!rdev->has_uvd)
>> +               return;
>> +
>> +       r = radeon_uvd_init(rdev);
>> +       if (r) {
>> +               dev_err(rdev->dev, "failed UVD (%d) init.\n", r);
>> +               /*
>> +                * At this point rdev->uvd.vcpu_bo is NULL which trickles
>> down
>> +                * to early fails uvd_v1_0_resume() and thus nothing
>> happens
>> +                * there. So it is pointless to try to go through that
>> code
>> +                * hence why we disable uvd here.
>> +                */
>> +               rdev->has_uvd = 0;
>> +               return;
>> +       }
>> +       rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_obj = NULL;
>> +       r600_ring_init(rdev, &rdev->ring[R600_RING_TYPE_UVD_INDEX], 4096);
>> +}
>> +
>> +static void r600_uvd_start(struct radeon_device *rdev)
>> +{
>> +       int r;
>> +
>> +       if (!rdev->has_uvd)
>> +               return;
>> +
>> +       r = uvd_v1_0_resume(rdev);
>> +       if (r) {
>> +               dev_err(rdev->dev, "failed UVD resume (%d).\n", r);
>> +               goto error;
>> +       }
>> +       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);
>> +               goto error;
>> +       }
>> +       return;
>> +
>> +error:
>> +       rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
>> +}
>> +
>> +static void r600_uvd_resume(struct radeon_device *rdev)
>> +{
>> +       struct radeon_ring *ring;
>> +       int r;
>> +
>> +       if (!rdev->has_uvd ||
>> !rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size)
>> +               return;
>> +
>> +       ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
>> +       r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
>> RADEON_CP_PACKET2);
>> +       if (r) {
>> +               dev_err(rdev->dev, "failed initializing UVD ring (%d).\n",
>> r);
>> +               return;
>> +       }
>> +       r = uvd_v1_0_init(rdev);
>> +       if (r) {
>> +               dev_err(rdev->dev, "failed initializing UVD (%d).\n", r);
>> +               return;
>> +       }
>> +}
>> +
>>   static int r600_startup(struct radeon_device *rdev)
>>   {
>>         struct radeon_ring *ring;
>> @@ -3070,17 +3137,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_start(rdev);
>>         /* Enable IRQ */
>>         if (!rdev->irq.installed) {
>> @@ -3110,17 +3167,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 +3311,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);
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff mbox

Patch

diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
index f86ab69..24fa982 100644
--- a/drivers/gpu/drm/radeon/r600.c
+++ b/drivers/gpu/drm/radeon/r600.c
@@ -3035,6 +3035,73 @@  void r600_clear_surface_reg(struct radeon_device *rdev, int reg)
 	/* FIXME: implement */
 }
 
+static void r600_uvd_init(struct radeon_device *rdev)
+{
+	int r;
+
+	if (!rdev->has_uvd)
+		return;
+
+	r = radeon_uvd_init(rdev);
+	if (r) {
+		dev_err(rdev->dev, "failed UVD (%d) init.\n", r);
+		/*
+		 * At this point rdev->uvd.vcpu_bo is NULL which trickles down
+		 * to early fails uvd_v1_0_resume() and thus nothing happens
+		 * there. So it is pointless to try to go through that code
+		 * hence why we disable uvd here.
+		 */
+		rdev->has_uvd = 0;
+		return;
+	}
+	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_obj = NULL;
+	r600_ring_init(rdev, &rdev->ring[R600_RING_TYPE_UVD_INDEX], 4096);
+}
+
+static void r600_uvd_start(struct radeon_device *rdev)
+{
+	int r;
+
+	if (!rdev->has_uvd)
+		return;
+
+	r = uvd_v1_0_resume(rdev);
+	if (r) {
+		dev_err(rdev->dev, "failed UVD resume (%d).\n", r);
+		goto error;
+	}
+	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);
+		goto error;
+	}
+	return;
+
+error:
+	rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
+}
+
+static void r600_uvd_resume(struct radeon_device *rdev)
+{
+	struct radeon_ring *ring;
+	int r;
+
+	if (!rdev->has_uvd || !rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size)
+		return;
+
+	ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
+	r = radeon_ring_init(rdev, ring, ring->ring_size, 0, RADEON_CP_PACKET2);
+	if (r) {
+		dev_err(rdev->dev, "failed initializing UVD ring (%d).\n", r);
+		return;
+	}
+	r = uvd_v1_0_init(rdev);
+	if (r) {
+		dev_err(rdev->dev, "failed initializing UVD (%d).\n", r);
+		return;
+	}
+}
+
 static int r600_startup(struct radeon_device *rdev)
 {
 	struct radeon_ring *ring;
@@ -3070,17 +3137,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_start(rdev);
 
 	/* Enable IRQ */
 	if (!rdev->irq.installed) {
@@ -3110,17 +3167,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 +3311,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);