drm/amdgpu: move some release handles into fail labels

Submitted by Huang, Ray on Sept. 12, 2016, 2:16 a.m.

Details

Message ID 1473646581-25186-1-git-send-email-ray.huang@amd.com
State New
Headers show
Series "drm/amdgpu: move some release handles into fail labels" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Huang, Ray Sept. 12, 2016, 2:16 a.m.
Clean up the codes to move the release handles into fail labels.

Signed-off-by: Huang Rui <ray.huang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
index 5cc95f1..4a5c211 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
@@ -247,35 +247,28 @@  int amdgpu_ucode_init_bo(struct amdgpu_device *adev)
 	const struct common_firmware_header *header = NULL;
 
 	err = amdgpu_bo_create(adev, adev->firmware.fw_size, PAGE_SIZE, true,
-			AMDGPU_GEM_DOMAIN_GTT, 0, NULL, NULL, bo);
+			       AMDGPU_GEM_DOMAIN_GTT, 0, NULL, NULL, bo);
 	if (err) {
 		dev_err(adev->dev, "(%d) Firmware buffer allocate failed\n", err);
-		err = -ENOMEM;
 		goto failed;
 	}
 
 	err = amdgpu_bo_reserve(*bo, false);
 	if (err) {
-		amdgpu_bo_unref(bo);
 		dev_err(adev->dev, "(%d) Firmware buffer reserve failed\n", err);
-		goto failed;
+		goto failed_reserve;
 	}
 
 	err = amdgpu_bo_pin(*bo, AMDGPU_GEM_DOMAIN_GTT, &fw_mc_addr);
 	if (err) {
-		amdgpu_bo_unreserve(*bo);
-		amdgpu_bo_unref(bo);
 		dev_err(adev->dev, "(%d) Firmware buffer pin failed\n", err);
-		goto failed;
+		goto failed_pin;
 	}
 
 	err = amdgpu_bo_kmap(*bo, &fw_buf_ptr);
 	if (err) {
 		dev_err(adev->dev, "(%d) Firmware buffer kmap failed\n", err);
-		amdgpu_bo_unpin(*bo);
-		amdgpu_bo_unreserve(*bo);
-		amdgpu_bo_unref(bo);
-		goto failed;
+		goto failed_kmap;
 	}
 
 	amdgpu_bo_unreserve(*bo);
@@ -291,6 +284,12 @@  int amdgpu_ucode_init_bo(struct amdgpu_device *adev)
 		}
 	}
 
+failed_kmap:
+	amdgpu_bo_unpin(*bo);
+failed_pin:
+	amdgpu_bo_unreserve(*bo);
+failed_reserve:
+	amdgpu_bo_unref(bo);
 failed:
 	if (err)
 		adev->firmware.smu_load = false;

Comments

On 2016年09月12日 10:16, Huang Rui wrote:
> Clean up the codes to move the release handles into fail labels.
>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
Reviewed-by: Chunming Zhou <david1.zhou@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c | 21 ++++++++++-----------
>   1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> index 5cc95f1..4a5c211 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> @@ -247,35 +247,28 @@ int amdgpu_ucode_init_bo(struct amdgpu_device *adev)
>   	const struct common_firmware_header *header = NULL;
>   
>   	err = amdgpu_bo_create(adev, adev->firmware.fw_size, PAGE_SIZE, true,
> -			AMDGPU_GEM_DOMAIN_GTT, 0, NULL, NULL, bo);
> +			       AMDGPU_GEM_DOMAIN_GTT, 0, NULL, NULL, bo);
>   	if (err) {
>   		dev_err(adev->dev, "(%d) Firmware buffer allocate failed\n", err);
> -		err = -ENOMEM;
>   		goto failed;
>   	}
>   
>   	err = amdgpu_bo_reserve(*bo, false);
>   	if (err) {
> -		amdgpu_bo_unref(bo);
>   		dev_err(adev->dev, "(%d) Firmware buffer reserve failed\n", err);
> -		goto failed;
> +		goto failed_reserve;
>   	}
>   
>   	err = amdgpu_bo_pin(*bo, AMDGPU_GEM_DOMAIN_GTT, &fw_mc_addr);
>   	if (err) {
> -		amdgpu_bo_unreserve(*bo);
> -		amdgpu_bo_unref(bo);
>   		dev_err(adev->dev, "(%d) Firmware buffer pin failed\n", err);
> -		goto failed;
> +		goto failed_pin;
>   	}
>   
>   	err = amdgpu_bo_kmap(*bo, &fw_buf_ptr);
>   	if (err) {
>   		dev_err(adev->dev, "(%d) Firmware buffer kmap failed\n", err);
> -		amdgpu_bo_unpin(*bo);
> -		amdgpu_bo_unreserve(*bo);
> -		amdgpu_bo_unref(bo);
> -		goto failed;
> +		goto failed_kmap;
>   	}
>   
>   	amdgpu_bo_unreserve(*bo);
> @@ -291,6 +284,12 @@ int amdgpu_ucode_init_bo(struct amdgpu_device *adev)
>   		}
>   	}
>   
> +failed_kmap:
> +	amdgpu_bo_unpin(*bo);
> +failed_pin:
> +	amdgpu_bo_unreserve(*bo);
> +failed_reserve:
> +	amdgpu_bo_unref(bo);
>   failed:
>   	if (err)
>   		adev->firmware.smu_load = false;
Am 12.09.2016 um 04:16 schrieb Huang Rui:
> Clean up the codes to move the release handles into fail labels.
>
> Signed-off-by: Huang Rui <ray.huang@amd.com>

Reviewed-by: Christian König <christian.koenig@amd.com>.

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c | 21 ++++++++++-----------
>   1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> index 5cc95f1..4a5c211 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> @@ -247,35 +247,28 @@ int amdgpu_ucode_init_bo(struct amdgpu_device *adev)
>   	const struct common_firmware_header *header = NULL;
>   
>   	err = amdgpu_bo_create(adev, adev->firmware.fw_size, PAGE_SIZE, true,
> -			AMDGPU_GEM_DOMAIN_GTT, 0, NULL, NULL, bo);
> +			       AMDGPU_GEM_DOMAIN_GTT, 0, NULL, NULL, bo);
>   	if (err) {
>   		dev_err(adev->dev, "(%d) Firmware buffer allocate failed\n", err);
> -		err = -ENOMEM;
>   		goto failed;
>   	}
>   
>   	err = amdgpu_bo_reserve(*bo, false);
>   	if (err) {
> -		amdgpu_bo_unref(bo);
>   		dev_err(adev->dev, "(%d) Firmware buffer reserve failed\n", err);
> -		goto failed;
> +		goto failed_reserve;
>   	}
>   
>   	err = amdgpu_bo_pin(*bo, AMDGPU_GEM_DOMAIN_GTT, &fw_mc_addr);
>   	if (err) {
> -		amdgpu_bo_unreserve(*bo);
> -		amdgpu_bo_unref(bo);
>   		dev_err(adev->dev, "(%d) Firmware buffer pin failed\n", err);
> -		goto failed;
> +		goto failed_pin;
>   	}
>   
>   	err = amdgpu_bo_kmap(*bo, &fw_buf_ptr);
>   	if (err) {
>   		dev_err(adev->dev, "(%d) Firmware buffer kmap failed\n", err);
> -		amdgpu_bo_unpin(*bo);
> -		amdgpu_bo_unreserve(*bo);
> -		amdgpu_bo_unref(bo);
> -		goto failed;
> +		goto failed_kmap;
>   	}
>   
>   	amdgpu_bo_unreserve(*bo);
> @@ -291,6 +284,12 @@ int amdgpu_ucode_init_bo(struct amdgpu_device *adev)
>   		}
>   	}
>   
> +failed_kmap:
> +	amdgpu_bo_unpin(*bo);
> +failed_pin:
> +	amdgpu_bo_unreserve(*bo);
> +failed_reserve:
> +	amdgpu_bo_unref(bo);
>   failed:
>   	if (err)
>   		adev->firmware.smu_load = false;
Hi,

This patch causes a massive performance drop (by a factor of 100)
followed by GPU hangs with F1 2015 (it needs more than 3GB of memory,
the card has 2GB). The game normally works really well (30+ FPS on
Polaris11 2GB, 40+ FPS on Tonga 2GB).

It should be fixed or the commit should be reverted really quickly.

Marek


On Mon, Sep 12, 2016 at 4:16 AM, Huang Rui <ray.huang@amd.com> wrote:
> Clean up the codes to move the release handles into fail labels.
>
> Signed-off-by: Huang Rui <ray.huang@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> index 5cc95f1..4a5c211 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> @@ -247,35 +247,28 @@ int amdgpu_ucode_init_bo(struct amdgpu_device *adev)
>         const struct common_firmware_header *header = NULL;
>
>         err = amdgpu_bo_create(adev, adev->firmware.fw_size, PAGE_SIZE, true,
> -                       AMDGPU_GEM_DOMAIN_GTT, 0, NULL, NULL, bo);
> +                              AMDGPU_GEM_DOMAIN_GTT, 0, NULL, NULL, bo);
>         if (err) {
>                 dev_err(adev->dev, "(%d) Firmware buffer allocate failed\n", err);
> -               err = -ENOMEM;
>                 goto failed;
>         }
>
>         err = amdgpu_bo_reserve(*bo, false);
>         if (err) {
> -               amdgpu_bo_unref(bo);
>                 dev_err(adev->dev, "(%d) Firmware buffer reserve failed\n", err);
> -               goto failed;
> +               goto failed_reserve;
>         }
>
>         err = amdgpu_bo_pin(*bo, AMDGPU_GEM_DOMAIN_GTT, &fw_mc_addr);
>         if (err) {
> -               amdgpu_bo_unreserve(*bo);
> -               amdgpu_bo_unref(bo);
>                 dev_err(adev->dev, "(%d) Firmware buffer pin failed\n", err);
> -               goto failed;
> +               goto failed_pin;
>         }
>
>         err = amdgpu_bo_kmap(*bo, &fw_buf_ptr);
>         if (err) {
>                 dev_err(adev->dev, "(%d) Firmware buffer kmap failed\n", err);
> -               amdgpu_bo_unpin(*bo);
> -               amdgpu_bo_unreserve(*bo);
> -               amdgpu_bo_unref(bo);
> -               goto failed;
> +               goto failed_kmap;
>         }
>
>         amdgpu_bo_unreserve(*bo);
> @@ -291,6 +284,12 @@ int amdgpu_ucode_init_bo(struct amdgpu_device *adev)
>                 }
>         }
>
> +failed_kmap:
> +       amdgpu_bo_unpin(*bo);
> +failed_pin:
> +       amdgpu_bo_unreserve(*bo);
> +failed_reserve:
> +       amdgpu_bo_unref(bo);
>  failed:
>         if (err)
>                 adev->firmware.smu_load = false;
> --
> 2.7.4
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Thanks for that info, I was busy most of the day bisecting this 
regression as well.

Yeah, the bug is rather obvious, there is a "return 0;" in the good case 
missing here.

So after loading the firmware we release it again. I will hack together 
a patch, just give me a minute.

Christian.

Am 14.09.2016 um 16:21 schrieb Marek Olšák:
> Hi,
>
> This patch causes a massive performance drop (by a factor of 100)
> followed by GPU hangs with F1 2015 (it needs more than 3GB of memory,
> the card has 2GB). The game normally works really well (30+ FPS on
> Polaris11 2GB, 40+ FPS on Tonga 2GB).
>
> It should be fixed or the commit should be reverted really quickly.
>
> Marek
>
>
> On Mon, Sep 12, 2016 at 4:16 AM, Huang Rui <ray.huang@amd.com> wrote:
>> Clean up the codes to move the release handles into fail labels.
>>
>> Signed-off-by: Huang Rui <ray.huang@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c | 21 ++++++++++-----------
>>   1 file changed, 10 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
>> index 5cc95f1..4a5c211 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
>> @@ -247,35 +247,28 @@ int amdgpu_ucode_init_bo(struct amdgpu_device *adev)
>>          const struct common_firmware_header *header = NULL;
>>
>>          err = amdgpu_bo_create(adev, adev->firmware.fw_size, PAGE_SIZE, true,
>> -                       AMDGPU_GEM_DOMAIN_GTT, 0, NULL, NULL, bo);
>> +                              AMDGPU_GEM_DOMAIN_GTT, 0, NULL, NULL, bo);
>>          if (err) {
>>                  dev_err(adev->dev, "(%d) Firmware buffer allocate failed\n", err);
>> -               err = -ENOMEM;
>>                  goto failed;
>>          }
>>
>>          err = amdgpu_bo_reserve(*bo, false);
>>          if (err) {
>> -               amdgpu_bo_unref(bo);
>>                  dev_err(adev->dev, "(%d) Firmware buffer reserve failed\n", err);
>> -               goto failed;
>> +               goto failed_reserve;
>>          }
>>
>>          err = amdgpu_bo_pin(*bo, AMDGPU_GEM_DOMAIN_GTT, &fw_mc_addr);
>>          if (err) {
>> -               amdgpu_bo_unreserve(*bo);
>> -               amdgpu_bo_unref(bo);
>>                  dev_err(adev->dev, "(%d) Firmware buffer pin failed\n", err);
>> -               goto failed;
>> +               goto failed_pin;
>>          }
>>
>>          err = amdgpu_bo_kmap(*bo, &fw_buf_ptr);
>>          if (err) {
>>                  dev_err(adev->dev, "(%d) Firmware buffer kmap failed\n", err);
>> -               amdgpu_bo_unpin(*bo);
>> -               amdgpu_bo_unreserve(*bo);
>> -               amdgpu_bo_unref(bo);
>> -               goto failed;
>> +               goto failed_kmap;
>>          }
>>
>>          amdgpu_bo_unreserve(*bo);
>> @@ -291,6 +284,12 @@ int amdgpu_ucode_init_bo(struct amdgpu_device *adev)
>>                  }
>>          }
>>
>> +failed_kmap:
>> +       amdgpu_bo_unpin(*bo);
>> +failed_pin:
>> +       amdgpu_bo_unreserve(*bo);
>> +failed_reserve:
>> +       amdgpu_bo_unref(bo);
>>   failed:
>>          if (err)
>>                  adev->firmware.smu_load = false;
>> --
>> 2.7.4
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Ah, sorry, this is my mistake to miss the "return 0;". Marek, thanks a lot
to point out. I will take care next time.

Christian, I see you already send the patch to fix it. Thanks.

Thanks,
Rui

On Wed, Sep 14, 2016 at 10:36:16PM +0800, Koenig, Christian wrote:
> Thanks for that info, I was busy most of the day bisecting this 
> regression as well.
> 
> Yeah, the bug is rather obvious, there is a "return 0;" in the good case 
> missing here.
> 
> So after loading the firmware we release it again. I will hack together 
> a patch, just give me a minute.
> 
> Christian.
> 
> Am 14.09.2016 um 16:21 schrieb Marek Olšák:
> > Hi,
> >
> > This patch causes a massive performance drop (by a factor of 100)
> > followed by GPU hangs with F1 2015 (it needs more than 3GB of memory,
> > the card has 2GB). The game normally works really well (30+ FPS on
> > Polaris11 2GB, 40+ FPS on Tonga 2GB).
> >
> > It should be fixed or the commit should be reverted really quickly.
> >
> > Marek
> >
> >
> > On Mon, Sep 12, 2016 at 4:16 AM, Huang Rui <ray.huang@amd.com> wrote:
> >> Clean up the codes to move the release handles into fail labels.
> >>
> >> Signed-off-by: Huang Rui <ray.huang@amd.com>
> >> ---
> >>   drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c | 21 ++++++++++-----------
> >>   1 file changed, 10 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> >> index 5cc95f1..4a5c211 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> >> @@ -247,35 +247,28 @@ int amdgpu_ucode_init_bo(struct amdgpu_device *adev)
> >>          const struct common_firmware_header *header = NULL;
> >>
> >>          err = amdgpu_bo_create(adev, adev->firmware.fw_size, PAGE_SIZE, true,
> >> -                       AMDGPU_GEM_DOMAIN_GTT, 0, NULL, NULL, bo);
> >> +                              AMDGPU_GEM_DOMAIN_GTT, 0, NULL, NULL, bo);
> >>          if (err) {
> >>                  dev_err(adev->dev, "(%d) Firmware buffer allocate failed\n", err);
> >> -               err = -ENOMEM;
> >>                  goto failed;
> >>          }
> >>
> >>          err = amdgpu_bo_reserve(*bo, false);
> >>          if (err) {
> >> -               amdgpu_bo_unref(bo);
> >>                  dev_err(adev->dev, "(%d) Firmware buffer reserve failed\n", err);
> >> -               goto failed;
> >> +               goto failed_reserve;
> >>          }
> >>
> >>          err = amdgpu_bo_pin(*bo, AMDGPU_GEM_DOMAIN_GTT, &fw_mc_addr);
> >>          if (err) {
> >> -               amdgpu_bo_unreserve(*bo);
> >> -               amdgpu_bo_unref(bo);
> >>                  dev_err(adev->dev, "(%d) Firmware buffer pin failed\n", err);
> >> -               goto failed;
> >> +               goto failed_pin;
> >>          }
> >>
> >>          err = amdgpu_bo_kmap(*bo, &fw_buf_ptr);
> >>          if (err) {
> >>                  dev_err(adev->dev, "(%d) Firmware buffer kmap failed\n", err);
> >> -               amdgpu_bo_unpin(*bo);
> >> -               amdgpu_bo_unreserve(*bo);
> >> -               amdgpu_bo_unref(bo);
> >> -               goto failed;
> >> +               goto failed_kmap;
> >>          }
> >>
> >>          amdgpu_bo_unreserve(*bo);
> >> @@ -291,6 +284,12 @@ int amdgpu_ucode_init_bo(struct amdgpu_device *adev)
> >>                  }
> >>          }
> >>
> >> +failed_kmap:
> >> +       amdgpu_bo_unpin(*bo);
> >> +failed_pin:
> >> +       amdgpu_bo_unreserve(*bo);
> >> +failed_reserve:
> >> +       amdgpu_bo_unref(bo);
> >>   failed:
> >>          if (err)
> >>                  adev->firmware.smu_load = false;
> >> --
> >> 2.7.4
> >>
> >> _______________________________________________
> >> amd-gfx mailing list
> >> amd-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 
>