Re: [PATCH 1/2] drm/amdgpu/virtual_dce: No need to pin the fb's bo

[Christian König]

Am 27.12.18 um 15:30 schrieb Michel Dänzer:

On 2018-12-24 7:08 a.m., Emily Deng wrote:

For virtual display, no need to pin the fb's bo.

Signed-off-by: Emily Deng 

[...]

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index ea89c54..4e94473 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -188,10 +188,12 @@ int amdgpu_display_crtc_page_flip_target(struct drm_crtc 
*crtc,
goto cleanup;
}
  
-	r = amdgpu_bo_pin(new_abo, amdgpu_display_supported_domains(adev));

-   if (unlikely(r != 0)) {
-   DRM_ERROR("failed to pin new abo buffer before flip\n");
-   goto unreserve;
+   if (!adev->enable_virtual_display) {
+   r = amdgpu_bo_pin(new_abo, 
amdgpu_display_supported_domains(adev));
+   if (unlikely(r != 0)) {
+   DRM_ERROR("failed to pin new abo buffer before flip\n");
+   goto unreserve;
+   }
}
  
  	r = amdgpu_ttm_alloc_gart(_abo->tbo);

I think the amdgpu_ttm_alloc_gart call could be skipped as well?


Yup agree.





@@ -242,9 +245,10 @@ int amdgpu_display_crtc_page_flip_target(struct drm_crtc 
*crtc,
goto cleanup;
}
  unpin:
-   if (unlikely(amdgpu_bo_unpin(new_abo) != 0)) {
-   DRM_ERROR("failed to unpin new abo in error path\n");
-   }
+   if (!adev->enable_virtual_display)
+   if (unlikely(amdgpu_bo_unpin(new_abo) != 0))
+   DRM_ERROR("failed to unpin new abo in error path\n");

This should be written as either

if (!adev->enable_virtual_display &&
unlikely(amdgpu_bo_unpin(new_abo) != 0))
DRM_ERROR("failed to unpin new abo in error path\n");

or

if (!adev->enable_virtual_display) {
if (unlikely(amdgpu_bo_unpin(new_abo) != 0))
DRM_ERROR("failed to unpin new abo in error path\n");
}


P.S. A lot of people are out for Christmas and New Year's.


Indeed and thanks for the good explanation of the background in the 
other mail. I didn't realized that this was actually a bug fix for a pin 
imbalance.


BTW: It's good practice to note information like that in the commit message.

Anyway with the comment above fixed the series is Acked-by: Christian 
König 


Christian.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amdgpu/virtual_dce: No need to pin the fb's bo

[Michel Dänzer]

On 2018-12-24 7:08 a.m., Emily Deng wrote:
> For virtual display, no need to pin the fb's bo.
> 
> Signed-off-by: Emily Deng 
> 
> [...]
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index ea89c54..4e94473 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -188,10 +188,12 @@ int amdgpu_display_crtc_page_flip_target(struct 
> drm_crtc *crtc,
>   goto cleanup;
>   }
>  
> - r = amdgpu_bo_pin(new_abo, amdgpu_display_supported_domains(adev));
> - if (unlikely(r != 0)) {
> - DRM_ERROR("failed to pin new abo buffer before flip\n");
> - goto unreserve;
> + if (!adev->enable_virtual_display) {
> + r = amdgpu_bo_pin(new_abo, 
> amdgpu_display_supported_domains(adev));
> + if (unlikely(r != 0)) {
> + DRM_ERROR("failed to pin new abo buffer before flip\n");
> + goto unreserve;
> + }
>   }
>  
>   r = amdgpu_ttm_alloc_gart(_abo->tbo);

I think the amdgpu_ttm_alloc_gart call could be skipped as well?


> @@ -242,9 +245,10 @@ int amdgpu_display_crtc_page_flip_target(struct drm_crtc 
> *crtc,
>   goto cleanup;
>   }
>  unpin:
> - if (unlikely(amdgpu_bo_unpin(new_abo) != 0)) {
> - DRM_ERROR("failed to unpin new abo in error path\n");
> - }
> + if (!adev->enable_virtual_display)
> + if (unlikely(amdgpu_bo_unpin(new_abo) != 0))
> + DRM_ERROR("failed to unpin new abo in error path\n");

This should be written as either

if (!adev->enable_virtual_display &&
unlikely(amdgpu_bo_unpin(new_abo) != 0))
DRM_ERROR("failed to unpin new abo in error path\n");

or

if (!adev->enable_virtual_display) {
if (unlikely(amdgpu_bo_unpin(new_abo) != 0))
DRM_ERROR("failed to unpin new abo in error path\n");
}


P.S. A lot of people are out for Christmas and New Year's.

-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx