[PATCH v5 0/4] Enable S/G for Picasso

[PATCH v5 0/4] Enable S/G for Picasso

[Andrey Grodzovsky]

First patches fixes a hard hang introduced by placing the display BO in 
GTT memory because of HW issues with cached mappings. Second patch does
some minor reafactoring to resue code in thrid patch. Third patch adds
check for USWC support as condition to placing APUs scanout BO in GTT.
Last patch enables S/G.

Andrey Grodzovsky (3):
  drm/amdgpu: Fix hard hang for S/G display BOs.
  drm/amdgpu: Create helper to clear AMDGPU_GEM_CREATE_CPU_GTT_USWC
  drm/amdgpu: Add check for USWC support for
    amdgpu_display_supported_domains

Shirish S (1):
  drm/amd/display: enable S/G for RAVEN chip

 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c       | 11 +++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c            |  7 ++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c           |  3 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c        | 61 +++++++++++++----------
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.h        |  2 +
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 +-
 6 files changed, 52 insertions(+), 34 deletions(-)

-- 
2.7.4

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

[PATCH v5 1/4] drm/amdgpu: Fix hard hang for S/G display BOs.

[Andrey Grodzovsky]

HW requires for caching to be unset for scanout BO
mappings when the BO placement is in GTT memory.
Usually the flag to unset is passed from user mode
but for FB mode this was missing.

v2:
Keep all BO placement logic in amdgpu_display_supported_domains

Suggested-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Tested-by: Shirish S <shirish.s@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c  | 7 +++----
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 3 ++-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
index e476092..bf0c61b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
@@ -137,14 +137,14 @@ static int amdgpufb_create_pinned_object(struct amdgpu_fbdev *rfbdev,
 	mode_cmd->pitches[0] = amdgpu_align_pitch(adev, mode_cmd->width, cpp,
 						  fb_tiled);
 	domain = amdgpu_display_supported_domains(adev);
-
 	height = ALIGN(mode_cmd->height, 8);
 	size = mode_cmd->pitches[0] * height;
 	aligned_size = ALIGN(size, PAGE_SIZE);
 	ret = amdgpu_gem_object_create(adev, aligned_size, 0, domain,
 				       AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED |
-				       AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS |
-				       AMDGPU_GEM_CREATE_VRAM_CLEARED,
+				       AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS     |
+				       AMDGPU_GEM_CREATE_VRAM_CLEARED 	     |
+				       AMDGPU_GEM_CREATE_CPU_GTT_USWC,
 				       ttm_bo_type_kernel, NULL, &gobj);
 	if (ret) {
 		pr_err("failed to allocate framebuffer (%d)\n", aligned_size);
@@ -166,7 +166,6 @@ static int amdgpufb_create_pinned_object(struct amdgpu_fbdev *rfbdev,
 			dev_err(adev->dev, "FB failed to set tiling flags\n");
 	}
 
-
 	ret = amdgpu_bo_pin(abo, domain);
 	if (ret) {
 		amdgpu_bo_unreserve(abo);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 2cead5a..eeed089 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -743,7 +743,8 @@ int amdgpu_mode_dumb_create(struct drm_file *file_priv,
 	struct amdgpu_device *adev = dev->dev_private;
 	struct drm_gem_object *gobj;
 	uint32_t handle;
-	u64 flags = AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
+	u64 flags = AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED |
+		    AMDGPU_GEM_CREATE_CPU_GTT_USWC;
 	u32 domain;
 	int r;
 
-- 
2.7.4

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

[PATCH v5 2/4] drm/amdgpu: Create helper to clear AMDGPU_GEM_CREATE_CPU_GTT_USWC

[Andrey Grodzovsky]

Move the logic to clear AMDGPU_GEM_CREATE_CPU_GTT_USWC in
amdgpu_bo_do_create into standalone helper so it can be reused
in other functions.

v4:
Switch to return bool.

v5: Fix typos.

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 61 +++++++++++++++++-------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  2 +
 2 files changed, 37 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 989b7b5..8702062 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -413,6 +413,40 @@ static bool amdgpu_bo_validate_size(struct amdgpu_device *adev,
 	return false;
 }
 
+bool amdgpu_bo_support_uswc(u64 bo_flags)
+{
+
+#ifdef CONFIG_X86_32
+	/* XXX: Write-combined CPU mappings of GTT seem broken on 32-bit
+	 * See https://bugs.freedesktop.org/show_bug.cgi?id=84627
+	 */
+	return false;
+#elif defined(CONFIG_X86) && !defined(CONFIG_X86_PAT)
+	/* Don't try to enable write-combining when it can't work, or things
+	 * may be slow
+	 * See https://bugs.freedesktop.org/show_bug.cgi?id=88758
+	 */
+
+#ifndef CONFIG_COMPILE_TEST
+#warning Please enable CONFIG_MTRR and CONFIG_X86_PAT for better performance \
+	 thanks to write-combining
+#endif
+
+	if (bo_flags & AMDGPU_GEM_CREATE_CPU_GTT_USWC)
+		DRM_INFO_ONCE("Please enable CONFIG_MTRR and CONFIG_X86_PAT for "
+			      "better performance thanks to write-combining\n");
+	return false;
+#else
+	/* For architectures that don't support WC memory,
+	 * mask out the WC flag from the BO
+	 */
+	if (!drm_arch_can_wc_memory())
+		return false;
+
+	return true;
+#endif
+}
+
 static int amdgpu_bo_do_create(struct amdgpu_device *adev,
 			       struct amdgpu_bo_param *bp,
 			       struct amdgpu_bo **bo_ptr)
@@ -466,33 +500,8 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev,
 
 	bo->flags = bp->flags;
 
-#ifdef CONFIG_X86_32
-	/* XXX: Write-combined CPU mappings of GTT seem broken on 32-bit
-	 * See https://bugs.freedesktop.org/show_bug.cgi?id=84627
-	 */
-	bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC;
-#elif defined(CONFIG_X86) && !defined(CONFIG_X86_PAT)
-	/* Don't try to enable write-combining when it can't work, or things
-	 * may be slow
-	 * See https://bugs.freedesktop.org/show_bug.cgi?id=88758
-	 */
-
-#ifndef CONFIG_COMPILE_TEST
-#warning Please enable CONFIG_MTRR and CONFIG_X86_PAT for better performance \
-	 thanks to write-combining
-#endif
-
-	if (bo->flags & AMDGPU_GEM_CREATE_CPU_GTT_USWC)
-		DRM_INFO_ONCE("Please enable CONFIG_MTRR and CONFIG_X86_PAT for "
-			      "better performance thanks to write-combining\n");
-	bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC;
-#else
-	/* For architectures that don't support WC memory,
-	 * mask out the WC flag from the BO
-	 */
-	if (!drm_arch_can_wc_memory())
+	if (!amdgpu_bo_support_uswc(bo->flags))
 		bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC;
-#endif
 
 	bo->tbo.bdev = &adev->mman.bdev;
 	if (bp->domain & (AMDGPU_GEM_DOMAIN_GWS | AMDGPU_GEM_DOMAIN_OA |
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
index d60593c..dc44cf3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
@@ -308,5 +308,7 @@ void amdgpu_sa_bo_dump_debug_info(struct amdgpu_sa_manager *sa_manager,
 					 struct seq_file *m);
 #endif
 
+bool amdgpu_bo_support_uswc(u64 bo_flags);
+
 
 #endif
-- 
2.7.4

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

[PATCH v5 3/4] drm/amdgpu: Add check for USWC support for amdgpu_display_supported_domains

[Andrey Grodzovsky]

This verifies we don't add GTT as allowed domnain for APUs when USWC
is disabled.

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 767ee699..cac9975 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -498,8 +498,15 @@ uint32_t amdgpu_display_supported_domains(struct amdgpu_device *adev)
 	uint32_t domain = AMDGPU_GEM_DOMAIN_VRAM;
 
 #if defined(CONFIG_DRM_AMD_DC)
+	/*
+	 * if amdgpu_bo_validate_uswc returns false it means that USWC mappings
+	 * is not supported for this board. But this mapping is required
+	 * to avoid hang caused by placement of scanout BO in GTT on certain
+	 * APUs. So force the BO placement to VRAM in case this architecture
+	 * will not allow USWC mappings.
+	 */
 	if (adev->asic_type >= CHIP_CARRIZO && adev->asic_type < CHIP_RAVEN &&
-	    adev->flags & AMD_IS_APU &&
+	    adev->flags & AMD_IS_APU && amdgpu_bo_support_uswc(0) &&
 	    amdgpu_device_asic_has_dc_support(adev->asic_type))
 		domain |= AMDGPU_GEM_DOMAIN_GTT;
 #endif
-- 
2.7.4

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

[PATCH v5 4/4] drm/amd/display: enable S/G for RAVEN chip

[Andrey Grodzovsky]

From: Shirish S <shirish.s@amd.com>

enables gpu_vm_support in dm and adds
AMDGPU_GEM_DOMAIN_GTT as supported domain

v2:
Move BO placement logic into amdgpu_display_supported_domains

v3:
Use amdgpu_bo_validate_uswc in amdgpu_display_supported_domains.

v4:
amdgpu_bo_validate_uswc moved to sepperate patch.

Change-Id: If34300beaa60be2d36170b7b5b096ec644502b20
Signed-off-by: Shirish S <shirish.s@amd.com>
Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c       | 2 +-
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index cac9975..73045a3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -505,7 +505,7 @@ uint32_t amdgpu_display_supported_domains(struct amdgpu_device *adev)
 	 * APUs. So force the BO placement to VRAM in case this architecture
 	 * will not allow USWC mappings.
 	 */
-	if (adev->asic_type >= CHIP_CARRIZO && adev->asic_type < CHIP_RAVEN &&
+	if (adev->asic_type >= CHIP_CARRIZO && adev->asic_type <= CHIP_RAVEN &&
 	    adev->flags & AMD_IS_APU && amdgpu_bo_support_uswc(0) &&
 	    amdgpu_device_asic_has_dc_support(adev->asic_type))
 		domain |= AMDGPU_GEM_DOMAIN_GTT;
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 4922589..f0387ce1 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -686,7 +686,7 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
 	 */
 	if (adev->flags & AMD_IS_APU &&
 	    adev->asic_type >= CHIP_CARRIZO &&
-	    adev->asic_type < CHIP_RAVEN)
+	    adev->asic_type <= CHIP_RAVEN)
 		init_data.flags.gpu_vm_support = true;
 
 	if (amdgpu_dc_feature_mask & DC_FBC_MASK)
-- 
2.7.4

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

Re: [PATCH v5 4/4] drm/amd/display: enable S/G for RAVEN chip

[Kazlauskas, Nicholas]

On 7/25/19 10:24 AM, Andrey Grodzovsky wrote:
> From: Shirish S <shirish.s@amd.com>
> 
> enables gpu_vm_support in dm and adds
> AMDGPU_GEM_DOMAIN_GTT as supported domain
> 
> v2:
> Move BO placement logic into amdgpu_display_supported_domains
> 
> v3:
> Use amdgpu_bo_validate_uswc in amdgpu_display_supported_domains.
> 
> v4:
> amdgpu_bo_validate_uswc moved to sepperate patch.
> 
> Change-Id: If34300beaa60be2d36170b7b5b096ec644502b20
> Signed-off-by: Shirish S <shirish.s@amd.com>
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> ---

Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>

>   drivers/gpu/drm/amd/amdgpu/amdgpu_display.c       | 2 +-
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index cac9975..73045a3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -505,7 +505,7 @@ uint32_t amdgpu_display_supported_domains(struct amdgpu_device *adev)
>   	 * APUs. So force the BO placement to VRAM in case this architecture
>   	 * will not allow USWC mappings.
>   	 */
> -	if (adev->asic_type >= CHIP_CARRIZO && adev->asic_type < CHIP_RAVEN &&
> +	if (adev->asic_type >= CHIP_CARRIZO && adev->asic_type <= CHIP_RAVEN &&
>   	    adev->flags & AMD_IS_APU && amdgpu_bo_support_uswc(0) &&
>   	    amdgpu_device_asic_has_dc_support(adev->asic_type))
>   		domain |= AMDGPU_GEM_DOMAIN_GTT;
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 4922589..f0387ce1 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -686,7 +686,7 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
>   	 */
>   	if (adev->flags & AMD_IS_APU &&
>   	    adev->asic_type >= CHIP_CARRIZO &&
> -	    adev->asic_type < CHIP_RAVEN)
> +	    adev->asic_type <= CHIP_RAVEN)
>   		init_data.flags.gpu_vm_support = true;
>   
>   	if (amdgpu_dc_feature_mask & DC_FBC_MASK)
> 

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

Re: [PATCH v5 0/4] Enable S/G for Picasso

[Michel Dänzer]

On 2019-07-25 4:24 p.m., Andrey Grodzovsky wrote:
> First patches fixes a hard hang introduced by placing the display BO in 
> GTT memory because of HW issues with cached mappings. Second patch does
> some minor reafactoring to resue code in thrid patch. Third patch adds
> check for USWC support as condition to placing APUs scanout BO in GTT.
> Last patch enables S/G.
> 
> Andrey Grodzovsky (3):
>   drm/amdgpu: Fix hard hang for S/G display BOs.
>   drm/amdgpu: Create helper to clear AMDGPU_GEM_CREATE_CPU_GTT_USWC
>   drm/amdgpu: Add check for USWC support for
>     amdgpu_display_supported_domains
> 
> Shirish S (1):
>   drm/amd/display: enable S/G for RAVEN chip

Patches 2 & 3 are

Acked-by: Michel Dänzer <michel.daenzer@amd.com>


-- 
Earthling Michel Dänzer               |              https://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

Re: [PATCH v5 2/4] drm/amdgpu: Create helper to clear AMDGPU_GEM_CREATE_CPU_GTT_USWC

[Christian König]

Am 25.07.19 um 16:24 schrieb Andrey Grodzovsky:
> Move the logic to clear AMDGPU_GEM_CREATE_CPU_GTT_USWC in
> amdgpu_bo_do_create into standalone helper so it can be reused
> in other functions.
>
> v4:
> Switch to return bool.
>
> v5: Fix typos.
>
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 61 +++++++++++++++++-------------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  2 +
>   2 files changed, 37 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 989b7b5..8702062 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -413,6 +413,40 @@ static bool amdgpu_bo_validate_size(struct amdgpu_device *adev,
>   	return false;
>   }
>   
> +bool amdgpu_bo_support_uswc(u64 bo_flags)
> +{
> +
> +#ifdef CONFIG_X86_32
> +	/* XXX: Write-combined CPU mappings of GTT seem broken on 32-bit
> +	 * See https://bugs.freedesktop.org/show_bug.cgi?id=84627
> +	 */
> +	return false;
> +#elif defined(CONFIG_X86) && !defined(CONFIG_X86_PAT)
> +	/* Don't try to enable write-combining when it can't work, or things
> +	 * may be slow
> +	 * See https://bugs.freedesktop.org/show_bug.cgi?id=88758
> +	 */
> +
> +#ifndef CONFIG_COMPILE_TEST
> +#warning Please enable CONFIG_MTRR and CONFIG_X86_PAT for better performance \
> +	 thanks to write-combining
> +#endif
> +
> +	if (bo_flags & AMDGPU_GEM_CREATE_CPU_GTT_USWC)
> +		DRM_INFO_ONCE("Please enable CONFIG_MTRR and CONFIG_X86_PAT for "
> +			      "better performance thanks to write-combining\n");

I don't think this message belongs here.

> +	return false;
> +#else
> +	/* For architectures that don't support WC memory,
> +	 * mask out the WC flag from the BO
> +	 */
> +	if (!drm_arch_can_wc_memory())
> +		return false;
> +
> +	return true;
> +#endif
> +}
> +
>   static int amdgpu_bo_do_create(struct amdgpu_device *adev,
>   			       struct amdgpu_bo_param *bp,
>   			       struct amdgpu_bo **bo_ptr)
> @@ -466,33 +500,8 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev,
>   
>   	bo->flags = bp->flags;
>   
> -#ifdef CONFIG_X86_32
> -	/* XXX: Write-combined CPU mappings of GTT seem broken on 32-bit
> -	 * See https://bugs.freedesktop.org/show_bug.cgi?id=84627
> -	 */
> -	bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC;
> -#elif defined(CONFIG_X86) && !defined(CONFIG_X86_PAT)
> -	/* Don't try to enable write-combining when it can't work, or things
> -	 * may be slow
> -	 * See https://bugs.freedesktop.org/show_bug.cgi?id=88758
> -	 */
> -
> -#ifndef CONFIG_COMPILE_TEST
> -#warning Please enable CONFIG_MTRR and CONFIG_X86_PAT for better performance \
> -	 thanks to write-combining
> -#endif
> -
> -	if (bo->flags & AMDGPU_GEM_CREATE_CPU_GTT_USWC)
> -		DRM_INFO_ONCE("Please enable CONFIG_MTRR and CONFIG_X86_PAT for "
> -			      "better performance thanks to write-combining\n");
> -	bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC;
> -#else
> -	/* For architectures that don't support WC memory,
> -	 * mask out the WC flag from the BO
> -	 */
> -	if (!drm_arch_can_wc_memory())
> +	if (!amdgpu_bo_support_uswc(bo->flags))
>   		bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC;

Rather here we should do "if (bo_flags & AMDGPU_GEM_CREATE_CPU_GTT_USWC 
&& !amdgpu_bo_support_uswc())" and then clear the flag and also print 
the warning.

Apart from that the series looks good to me,
Christian.

> -#endif
>   
>   	bo->tbo.bdev = &adev->mman.bdev;
>   	if (bp->domain & (AMDGPU_GEM_DOMAIN_GWS | AMDGPU_GEM_DOMAIN_OA |
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> index d60593c..dc44cf3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> @@ -308,5 +308,7 @@ void amdgpu_sa_bo_dump_debug_info(struct amdgpu_sa_manager *sa_manager,
>   					 struct seq_file *m);
>   #endif
>   
> +bool amdgpu_bo_support_uswc(u64 bo_flags);
> +
>   
>   #endif

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

Re: [PATCH v5 4/4] drm/amd/display: enable S/G for RAVEN chip

[Christian König]

Am 25.07.19 um 16:24 schrieb Andrey Grodzovsky:
> From: Shirish S <shirish.s@amd.com>
>
> enables gpu_vm_support in dm and adds
> AMDGPU_GEM_DOMAIN_GTT as supported domain
>
> v2:
> Move BO placement logic into amdgpu_display_supported_domains
>
> v3:
> Use amdgpu_bo_validate_uswc in amdgpu_display_supported_domains.
>
> v4:
> amdgpu_bo_validate_uswc moved to sepperate patch.
>
> Change-Id: If34300beaa60be2d36170b7b5b096ec644502b20
> Signed-off-by: Shirish S <shirish.s@amd.com>
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_display.c       | 2 +-
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index cac9975..73045a3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -505,7 +505,7 @@ uint32_t amdgpu_display_supported_domains(struct amdgpu_device *adev)
>   	 * APUs. So force the BO placement to VRAM in case this architecture
>   	 * will not allow USWC mappings.
>   	 */
> -	if (adev->asic_type >= CHIP_CARRIZO && adev->asic_type < CHIP_RAVEN &&
> +	if (adev->asic_type >= CHIP_CARRIZO && adev->asic_type <= CHIP_RAVEN &&

This whole approach is incorrect since we don't check the flags of the 
actually BO used for scanout any more.

As I wrote before it is still perfectly possible and valid that 
userspace never sets this flag.

Regards,
Christian.

>   	    adev->flags & AMD_IS_APU && amdgpu_bo_support_uswc(0) &&
>   	    amdgpu_device_asic_has_dc_support(adev->asic_type))
>   		domain |= AMDGPU_GEM_DOMAIN_GTT;
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 4922589..f0387ce1 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -686,7 +686,7 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
>   	 */
>   	if (adev->flags & AMD_IS_APU &&
>   	    adev->asic_type >= CHIP_CARRIZO &&
> -	    adev->asic_type < CHIP_RAVEN)
> +	    adev->asic_type <= CHIP_RAVEN)
>   		init_data.flags.gpu_vm_support = true;
>   
>   	if (amdgpu_dc_feature_mask & DC_FBC_MASK)

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

Re: [PATCH v5 2/4] drm/amdgpu: Create helper to clear AMDGPU_GEM_CREATE_CPU_GTT_USWC

[Michel Dänzer]

On 2019-07-26 9:11 a.m., Christian König wrote:
> Am 25.07.19 um 16:24 schrieb Andrey Grodzovsky:
>> Move the logic to clear AMDGPU_GEM_CREATE_CPU_GTT_USWC in
>> amdgpu_bo_do_create into standalone helper so it can be reused
>> in other functions.
>>
>> v4:
>> Switch to return bool.
>>
>> v5: Fix typos.
>>
>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 61
>> +++++++++++++++++-------------
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  2 +
>>   2 files changed, 37 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> index 989b7b5..8702062 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> @@ -413,6 +413,40 @@ static bool amdgpu_bo_validate_size(struct
>> amdgpu_device *adev,
>>       return false;
>>   }
>>   +bool amdgpu_bo_support_uswc(u64 bo_flags)
>> +{
>> +
>> +#ifdef CONFIG_X86_32
>> +    /* XXX: Write-combined CPU mappings of GTT seem broken on 32-bit
>> +     * See https://bugs.freedesktop.org/show_bug.cgi?id=84627
>> +     */
>> +    return false;
>> +#elif defined(CONFIG_X86) && !defined(CONFIG_X86_PAT)
>> +    /* Don't try to enable write-combining when it can't work, or things
>> +     * may be slow
>> +     * See https://bugs.freedesktop.org/show_bug.cgi?id=88758
>> +     */
>> +
>> +#ifndef CONFIG_COMPILE_TEST
>> +#warning Please enable CONFIG_MTRR and CONFIG_X86_PAT for better
>> performance \
>> +     thanks to write-combining
>> +#endif
>> +
>> +    if (bo_flags & AMDGPU_GEM_CREATE_CPU_GTT_USWC)
>> +        DRM_INFO_ONCE("Please enable CONFIG_MTRR and CONFIG_X86_PAT
>> for "
>> +                  "better performance thanks to write-combining\n");
> 
> I don't think this message belongs here.
> 
> [...]
>> @@ -466,33 +500,8 @@ static int amdgpu_bo_do_create(struct
>> [...]
>> +    if (!amdgpu_bo_support_uswc(bo->flags))
>>           bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC;
> 
> Rather here we should do "if (bo_flags & AMDGPU_GEM_CREATE_CPU_GTT_USWC
> && !amdgpu_bo_support_uswc())" and then clear the flag and also print
> the warning.

That would require duplicating the CONFIG_X86_PAT related logic here as
well, which is a bit ugly.


-- 
Earthling Michel Dänzer               |              https://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

Re: [PATCH v5 4/4] drm/amd/display: enable S/G for RAVEN chip

[Michel Dänzer]

On 2019-07-26 9:14 a.m., Christian König wrote:
> Am 25.07.19 um 16:24 schrieb Andrey Grodzovsky:
>> From: Shirish S <shirish.s@amd.com>
>>
>> enables gpu_vm_support in dm and adds
>> AMDGPU_GEM_DOMAIN_GTT as supported domain
>>
>> v2:
>> Move BO placement logic into amdgpu_display_supported_domains
>>
>> v3:
>> Use amdgpu_bo_validate_uswc in amdgpu_display_supported_domains.
>>
>> v4:
>> amdgpu_bo_validate_uswc moved to sepperate patch.
>>
>> Change-Id: If34300beaa60be2d36170b7b5b096ec644502b20
>> Signed-off-by: Shirish S <shirish.s@amd.com>
>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_display.c       | 2 +-
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>> index cac9975..73045a3 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>> @@ -505,7 +505,7 @@ uint32_t amdgpu_display_supported_domains(struct
>> amdgpu_device *adev)
>>        * APUs. So force the BO placement to VRAM in case this
>> architecture
>>        * will not allow USWC mappings.
>>        */
>> -    if (adev->asic_type >= CHIP_CARRIZO && adev->asic_type <
>> CHIP_RAVEN &&
>> +    if (adev->asic_type >= CHIP_CARRIZO && adev->asic_type <=
>> CHIP_RAVEN &&
> 
> This whole approach is incorrect since we don't check the flags of the
> actually BO used for scanout any more.
> 
> As I wrote before it is still perfectly possible and valid that
> userspace never sets this flag.

Oh right, now I get what you meant before!

I guess amdgpu_display_supported_domains needs to take the BO flags as a
parameter, and also check that AMDGPU_GEM_CREATE_CPU_GTT_USWC is
actually set.


-- 
Earthling Michel Dänzer               |              https://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

Re: [PATCH v5 2/4] drm/amdgpu: Create helper to clear AMDGPU_GEM_CREATE_CPU_GTT_USWC

[Christian König]

Am 26.07.19 um 10:54 schrieb Michel Dänzer:
> On 2019-07-26 9:11 a.m., Christian König wrote:
>> Am 25.07.19 um 16:24 schrieb Andrey Grodzovsky:
>>> Move the logic to clear AMDGPU_GEM_CREATE_CPU_GTT_USWC in
>>> amdgpu_bo_do_create into standalone helper so it can be reused
>>> in other functions.
>>>
>>> v4:
>>> Switch to return bool.
>>>
>>> v5: Fix typos.
>>>
>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 61
>>> +++++++++++++++++-------------
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  2 +
>>>    2 files changed, 37 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> index 989b7b5..8702062 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> @@ -413,6 +413,40 @@ static bool amdgpu_bo_validate_size(struct
>>> amdgpu_device *adev,
>>>        return false;
>>>    }
>>>    +bool amdgpu_bo_support_uswc(u64 bo_flags)
>>> +{
>>> +
>>> +#ifdef CONFIG_X86_32
>>> +    /* XXX: Write-combined CPU mappings of GTT seem broken on 32-bit
>>> +     * See https://bugs.freedesktop.org/show_bug.cgi?id=84627
>>> +     */
>>> +    return false;
>>> +#elif defined(CONFIG_X86) && !defined(CONFIG_X86_PAT)
>>> +    /* Don't try to enable write-combining when it can't work, or things
>>> +     * may be slow
>>> +     * See https://bugs.freedesktop.org/show_bug.cgi?id=88758
>>> +     */
>>> +
>>> +#ifndef CONFIG_COMPILE_TEST
>>> +#warning Please enable CONFIG_MTRR and CONFIG_X86_PAT for better
>>> performance \
>>> +     thanks to write-combining
>>> +#endif
>>> +
>>> +    if (bo_flags & AMDGPU_GEM_CREATE_CPU_GTT_USWC)
>>> +        DRM_INFO_ONCE("Please enable CONFIG_MTRR and CONFIG_X86_PAT
>>> for "
>>> +                  "better performance thanks to write-combining\n");
>> I don't think this message belongs here.
>>
>> [...]
>>> @@ -466,33 +500,8 @@ static int amdgpu_bo_do_create(struct
>>> [...]
>>> +    if (!amdgpu_bo_support_uswc(bo->flags))
>>>            bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC;
>> Rather here we should do "if (bo_flags & AMDGPU_GEM_CREATE_CPU_GTT_USWC
>> && !amdgpu_bo_support_uswc())" and then clear the flag and also print
>> the warning.
> That would require duplicating the CONFIG_X86_PAT related logic here as
> well, which is a bit ugly.

Actually I would say we should drop this extra check and always emit a 
message that USWC is disabled for this platform.

We now need it for more than just better performance and it should be 
explicitly noted that this is not available in the logs.

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

Re: [PATCH v5 4/4] drm/amd/display: enable S/G for RAVEN chip

[Grodzovsky, Andrey]

Totally agree, me or Shirish will add a new patch for this as I've 
already commuted the series.

Andrey

On 7/26/19 5:03 AM, Michel Dänzer wrote:
> On 2019-07-26 9:14 a.m., Christian König wrote:
>> Am 25.07.19 um 16:24 schrieb Andrey Grodzovsky:
>>> From: Shirish S <shirish.s@amd.com>
>>>
>>> enables gpu_vm_support in dm and adds
>>> AMDGPU_GEM_DOMAIN_GTT as supported domain
>>>
>>> v2:
>>> Move BO placement logic into amdgpu_display_supported_domains
>>>
>>> v3:
>>> Use amdgpu_bo_validate_uswc in amdgpu_display_supported_domains.
>>>
>>> v4:
>>> amdgpu_bo_validate_uswc moved to sepperate patch.
>>>
>>> Change-Id: If34300beaa60be2d36170b7b5b096ec644502b20
>>> Signed-off-by: Shirish S <shirish.s@amd.com>
>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_display.c       | 2 +-
>>>    drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
>>>    2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>> index cac9975..73045a3 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>>> @@ -505,7 +505,7 @@ uint32_t amdgpu_display_supported_domains(struct
>>> amdgpu_device *adev)
>>>         * APUs. So force the BO placement to VRAM in case this
>>> architecture
>>>         * will not allow USWC mappings.
>>>         */
>>> -    if (adev->asic_type >= CHIP_CARRIZO && adev->asic_type <
>>> CHIP_RAVEN &&
>>> +    if (adev->asic_type >= CHIP_CARRIZO && adev->asic_type <=
>>> CHIP_RAVEN &&
>> This whole approach is incorrect since we don't check the flags of the
>> actually BO used for scanout any more.
>>
>> As I wrote before it is still perfectly possible and valid that
>> userspace never sets this flag.
> Oh right, now I get what you meant before!
>
> I guess amdgpu_display_supported_domains needs to take the BO flags as a
> parameter, and also check that AMDGPU_GEM_CREATE_CPU_GTT_USWC is
> actually set.
>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v5 2/4] drm/amdgpu: Create helper to clear AMDGPU_GEM_CREATE_CPU_GTT_USWC

[Michel Dänzer]

On 2019-07-26 1:55 p.m., Christian König wrote:
> Am 26.07.19 um 10:54 schrieb Michel Dänzer:
>> On 2019-07-26 9:11 a.m., Christian König wrote:
>>> Am 25.07.19 um 16:24 schrieb Andrey Grodzovsky:
>>>> Move the logic to clear AMDGPU_GEM_CREATE_CPU_GTT_USWC in
>>>> amdgpu_bo_do_create into standalone helper so it can be reused
>>>> in other functions.
>>>>
>>>> v4:
>>>> Switch to return bool.
>>>>
>>>> v5: Fix typos.
>>>>
>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 61
>>>> +++++++++++++++++-------------
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  2 +
>>>>    2 files changed, 37 insertions(+), 26 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> index 989b7b5..8702062 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> @@ -413,6 +413,40 @@ static bool amdgpu_bo_validate_size(struct
>>>> amdgpu_device *adev,
>>>>        return false;
>>>>    }
>>>>    +bool amdgpu_bo_support_uswc(u64 bo_flags)
>>>> +{
>>>> +
>>>> +#ifdef CONFIG_X86_32
>>>> +    /* XXX: Write-combined CPU mappings of GTT seem broken on 32-bit
>>>> +     * See https://bugs.freedesktop.org/show_bug.cgi?id=84627
>>>> +     */
>>>> +    return false;
>>>> +#elif defined(CONFIG_X86) && !defined(CONFIG_X86_PAT)
>>>> +    /* Don't try to enable write-combining when it can't work, or
>>>> things
>>>> +     * may be slow
>>>> +     * See https://bugs.freedesktop.org/show_bug.cgi?id=88758
>>>> +     */
>>>> +
>>>> +#ifndef CONFIG_COMPILE_TEST
>>>> +#warning Please enable CONFIG_MTRR and CONFIG_X86_PAT for better
>>>> performance \
>>>> +     thanks to write-combining
>>>> +#endif
>>>> +
>>>> +    if (bo_flags & AMDGPU_GEM_CREATE_CPU_GTT_USWC)
>>>> +        DRM_INFO_ONCE("Please enable CONFIG_MTRR and CONFIG_X86_PAT
>>>> for "
>>>> +                  "better performance thanks to write-combining\n");
>>> I don't think this message belongs here.
>>>
>>> [...]
>>>> @@ -466,33 +500,8 @@ static int amdgpu_bo_do_create(struct
>>>> [...]
>>>> +    if (!amdgpu_bo_support_uswc(bo->flags))
>>>>            bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC;
>>> Rather here we should do "if (bo_flags & AMDGPU_GEM_CREATE_CPU_GTT_USWC
>>> && !amdgpu_bo_support_uswc())" and then clear the flag and also print
>>> the warning.
>> That would require duplicating the CONFIG_X86_PAT related logic here as
>> well, which is a bit ugly.
> 
> Actually I would say we should drop this extra check and always emit a
> message that USWC is disabled for this platform.
> 
> We now need it for more than just better performance and it should be
> explicitly noted that this is not available in the logs.

A log message which doesn't explain why it's disabled / how to enable it
would probably cause us user support pain.


-- 
Earthling Michel Dänzer               |              https://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

Re: [PATCH v5 2/4] drm/amdgpu: Create helper to clear AMDGPU_GEM_CREATE_CPU_GTT_USWC

[Christian König]

Am 26.07.19 um 16:53 schrieb Michel Dänzer:
> On 2019-07-26 1:55 p.m., Christian König wrote:
>> Am 26.07.19 um 10:54 schrieb Michel Dänzer:
>>> On 2019-07-26 9:11 a.m., Christian König wrote:
>>>> Am 25.07.19 um 16:24 schrieb Andrey Grodzovsky:
>>>>> Move the logic to clear AMDGPU_GEM_CREATE_CPU_GTT_USWC in
>>>>> amdgpu_bo_do_create into standalone helper so it can be reused
>>>>> in other functions.
>>>>>
>>>>> v4:
>>>>> Switch to return bool.
>>>>>
>>>>> v5: Fix typos.
>>>>>
>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>>> ---
>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 61
>>>>> +++++++++++++++++-------------
>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  2 +
>>>>>     2 files changed, 37 insertions(+), 26 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>> index 989b7b5..8702062 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>> @@ -413,6 +413,40 @@ static bool amdgpu_bo_validate_size(struct
>>>>> amdgpu_device *adev,
>>>>>         return false;
>>>>>     }
>>>>>     +bool amdgpu_bo_support_uswc(u64 bo_flags)
>>>>> +{
>>>>> +
>>>>> +#ifdef CONFIG_X86_32
>>>>> +    /* XXX: Write-combined CPU mappings of GTT seem broken on 32-bit
>>>>> +     * See https://bugs.freedesktop.org/show_bug.cgi?id=84627
>>>>> +     */
>>>>> +    return false;
>>>>> +#elif defined(CONFIG_X86) && !defined(CONFIG_X86_PAT)
>>>>> +    /* Don't try to enable write-combining when it can't work, or
>>>>> things
>>>>> +     * may be slow
>>>>> +     * See https://bugs.freedesktop.org/show_bug.cgi?id=88758
>>>>> +     */
>>>>> +
>>>>> +#ifndef CONFIG_COMPILE_TEST
>>>>> +#warning Please enable CONFIG_MTRR and CONFIG_X86_PAT for better
>>>>> performance \
>>>>> +     thanks to write-combining
>>>>> +#endif
>>>>> +
>>>>> +    if (bo_flags & AMDGPU_GEM_CREATE_CPU_GTT_USWC)
>>>>> +        DRM_INFO_ONCE("Please enable CONFIG_MTRR and CONFIG_X86_PAT
>>>>> for "
>>>>> +                  "better performance thanks to write-combining\n");
>>>> I don't think this message belongs here.
>>>>
>>>> [...]
>>>>> @@ -466,33 +500,8 @@ static int amdgpu_bo_do_create(struct
>>>>> [...]
>>>>> +    if (!amdgpu_bo_support_uswc(bo->flags))
>>>>>             bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC;
>>>> Rather here we should do "if (bo_flags & AMDGPU_GEM_CREATE_CPU_GTT_USWC
>>>> && !amdgpu_bo_support_uswc())" and then clear the flag and also print
>>>> the warning.
>>> That would require duplicating the CONFIG_X86_PAT related logic here as
>>> well, which is a bit ugly.
>> Actually I would say we should drop this extra check and always emit a
>> message that USWC is disabled for this platform.
>>
>> We now need it for more than just better performance and it should be
>> explicitly noted that this is not available in the logs.
> A log message which doesn't explain why it's disabled / how to enable it
> would probably cause us user support pain.

Mhm, sounds like you didn't got what I wanted to say.

No log message was fine as long as USWC was only a performance 
optimization, but now it becomes mandatory for correct operation in some 
settings.

In other words in very low VRAM configurations it can be that we can't 
enable higher resolution because the kernel is not compiled with the 
necessary flags for USWC support.

Printing that when the driver loads sounds like the best place to me.

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

Re: [PATCH v5 2/4] drm/amdgpu: Create helper to clear AMDGPU_GEM_CREATE_CPU_GTT_USWC

[Michel Dänzer]

On 2019-07-26 6:02 p.m., Christian König wrote:
> Am 26.07.19 um 16:53 schrieb Michel Dänzer:
>> On 2019-07-26 1:55 p.m., Christian König wrote:
>>> Am 26.07.19 um 10:54 schrieb Michel Dänzer:
>>>> On 2019-07-26 9:11 a.m., Christian König wrote:
>>>>> Am 25.07.19 um 16:24 schrieb Andrey Grodzovsky:
>>>>>> Move the logic to clear AMDGPU_GEM_CREATE_CPU_GTT_USWC in
>>>>>> amdgpu_bo_do_create into standalone helper so it can be reused
>>>>>> in other functions.
>>>>>>
>>>>>> v4:
>>>>>> Switch to return bool.
>>>>>>
>>>>>> v5: Fix typos.
>>>>>>
>>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 61
>>>>>> +++++++++++++++++-------------
>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  2 +
>>>>>>     2 files changed, 37 insertions(+), 26 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>> index 989b7b5..8702062 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>> @@ -413,6 +413,40 @@ static bool amdgpu_bo_validate_size(struct
>>>>>> amdgpu_device *adev,
>>>>>>         return false;
>>>>>>     }
>>>>>>     +bool amdgpu_bo_support_uswc(u64 bo_flags)
>>>>>> +{
>>>>>> +
>>>>>> +#ifdef CONFIG_X86_32
>>>>>> +    /* XXX: Write-combined CPU mappings of GTT seem broken on 32-bit
>>>>>> +     * See https://bugs.freedesktop.org/show_bug.cgi?id=84627
>>>>>> +     */
>>>>>> +    return false;
>>>>>> +#elif defined(CONFIG_X86) && !defined(CONFIG_X86_PAT)
>>>>>> +    /* Don't try to enable write-combining when it can't work, or
>>>>>> things
>>>>>> +     * may be slow
>>>>>> +     * See https://bugs.freedesktop.org/show_bug.cgi?id=88758
>>>>>> +     */
>>>>>> +
>>>>>> +#ifndef CONFIG_COMPILE_TEST
>>>>>> +#warning Please enable CONFIG_MTRR and CONFIG_X86_PAT for better
>>>>>> performance \
>>>>>> +     thanks to write-combining
>>>>>> +#endif
>>>>>> +
>>>>>> +    if (bo_flags & AMDGPU_GEM_CREATE_CPU_GTT_USWC)
>>>>>> +        DRM_INFO_ONCE("Please enable CONFIG_MTRR and CONFIG_X86_PAT
>>>>>> for "
>>>>>> +                  "better performance thanks to write-combining\n");
>>>>> I don't think this message belongs here.
>>>>>
>>>>> [...]
>>>>>> @@ -466,33 +500,8 @@ static int amdgpu_bo_do_create(struct
>>>>>> [...]
>>>>>> +    if (!amdgpu_bo_support_uswc(bo->flags))
>>>>>>             bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC;
>>>>> Rather here we should do "if (bo_flags &
>>>>> AMDGPU_GEM_CREATE_CPU_GTT_USWC
>>>>> && !amdgpu_bo_support_uswc())" and then clear the flag and also print
>>>>> the warning.
>>>> That would require duplicating the CONFIG_X86_PAT related logic here as
>>>> well, which is a bit ugly.
>>> Actually I would say we should drop this extra check and always emit a
>>> message that USWC is disabled for this platform.
>>>
>>> We now need it for more than just better performance and it should be
>>> explicitly noted that this is not available in the logs.
>> A log message which doesn't explain why it's disabled / how to enable it
>> would probably cause us user support pain.
> 
> Mhm, sounds like you didn't got what I wanted to say.
> 
> No log message was fine as long as USWC was only a performance
> optimization, but now it becomes mandatory for correct operation in some
> settings.
> 
> In other words in very low VRAM configurations it can be that we can't
> enable higher resolution because the kernel is not compiled with the
> necessary flags for USWC support.

With an APU which supports scatter/gather scanout, sure.

> Printing that when the driver loads sounds like the best place to me.

Works for me, but it still needs to explain why it's disabled / how to
enable it... Something like "enable PAT" or "use a 64-bit kernel".


-- 
Earthling Michel Dänzer               |              https://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