[Intel-gfx] [PATCH 1/2] drm/i915/display: Add smem fallback allocation for dpt

[Intel-gfx] [PATCH 1/2] drm/i915/display: Add smem fallback allocation for dpt

[Juha-Pekka Heikkila]

Add fallback smem allocation for dpt if stolen memory allocation failed.

Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
---
 drivers/gpu/drm/i915/display/intel_dpt.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dpt.c b/drivers/gpu/drm/i915/display/intel_dpt.c
index fb0e7e79e0cd..10008699656e 100644
--- a/drivers/gpu/drm/i915/display/intel_dpt.c
+++ b/drivers/gpu/drm/i915/display/intel_dpt.c
@@ -10,6 +10,7 @@
 #include "intel_display_types.h"
 #include "intel_dpt.h"
 #include "intel_fb.h"
+#include "gem/i915_gem_internal.h"
 
 struct i915_dpt {
 	struct i915_address_space vm;
@@ -128,6 +129,10 @@ struct i915_vma *intel_dpt_pin(struct i915_address_space *vm)
 	void __iomem *iomem;
 	struct i915_gem_ww_ctx ww;
 	int err;
+	u64 pin_flags = 0;
+
+	if (!i915_gem_object_is_lmem(dpt->obj))
+		pin_flags |= PIN_MAPPABLE;
 
 	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
 	atomic_inc(&i915->gpu_error.pending_fb_pin);
@@ -138,7 +143,7 @@ struct i915_vma *intel_dpt_pin(struct i915_address_space *vm)
 			continue;
 
 		vma = i915_gem_object_ggtt_pin_ww(dpt->obj, &ww, NULL, 0, 4096,
-						  HAS_LMEM(i915) ? 0 : PIN_MAPPABLE);
+						  pin_flags);
 		if (IS_ERR(vma)) {
 			err = PTR_ERR(vma);
 			continue;
@@ -248,10 +253,13 @@ intel_dpt_create(struct intel_framebuffer *fb)
 
 	size = round_up(size * sizeof(gen8_pte_t), I915_GTT_PAGE_SIZE);
 
-	if (HAS_LMEM(i915))
-		dpt_obj = i915_gem_object_create_lmem(i915, size, I915_BO_ALLOC_CONTIGUOUS);
-	else
+	dpt_obj = i915_gem_object_create_lmem(i915, size, I915_BO_ALLOC_CONTIGUOUS);
+	if (IS_ERR(dpt_obj) && i915_ggtt_has_aperture(to_gt(i915)->ggtt))
 		dpt_obj = i915_gem_object_create_stolen(i915, size);
+	if (IS_ERR(dpt_obj) && !HAS_LMEM(i915)) {
+		drm_dbg_kms(&i915->drm, "Allocating dpt from smem\n");
+		dpt_obj = i915_gem_object_create_internal(i915, size);
+	}
 	if (IS_ERR(dpt_obj))
 		return ERR_CAST(dpt_obj);
 
-- 
2.25.1

[Intel-gfx] [PATCH 2/2] drm/i915: Fix i915_vma_pin_iomap()

[Juha-Pekka Heikkila]

From: CQ Tang <cq.tang@intel.com>

Display might allocate a smem object and call
i915_vma_pin_iomap(), the existing code will fail.

This fix was suggested by Chris P Wilson, that we pin
the smem with i915_gem_object_pin_map_unlocked().

Signed-off-by: CQ Tang <cq.tang@intel.com>
Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
Cc: Chris Wilson <chris.p.wilson@intel.com>
Cc: Jari Tahvanainen <jari.tahvanainen@intel.com>
---
 drivers/gpu/drm/i915/i915_vma.c | 34 ++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 162e8d83691b..8ce016ef3dba 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -550,13 +550,6 @@ void __iomem *i915_vma_pin_iomap(struct i915_vma *vma)
 	if (WARN_ON_ONCE(vma->obj->flags & I915_BO_ALLOC_GPU_ONLY))
 		return IO_ERR_PTR(-EINVAL);
 
-	if (!i915_gem_object_is_lmem(vma->obj)) {
-		if (GEM_WARN_ON(!i915_vma_is_map_and_fenceable(vma))) {
-			err = -ENODEV;
-			goto err;
-		}
-	}
-
 	GEM_BUG_ON(!i915_vma_is_ggtt(vma));
 	GEM_BUG_ON(!i915_vma_is_bound(vma, I915_VMA_GLOBAL_BIND));
 	GEM_BUG_ON(i915_vma_verify_bind_complete(vma));
@@ -572,17 +565,31 @@ void __iomem *i915_vma_pin_iomap(struct i915_vma *vma)
 		if (i915_gem_object_is_lmem(vma->obj))
 			ptr = i915_gem_object_lmem_io_map(vma->obj, 0,
 							  vma->obj->base.size);
-		else
+		else if (i915_vma_is_map_and_fenceable(vma))
 			ptr = io_mapping_map_wc(&i915_vm_to_ggtt(vma->vm)->iomap,
 						vma->node.start,
 						vma->node.size);
+		else {
+			ptr = (void __iomem *)
+				i915_gem_object_pin_map_unlocked(vma->obj,
+								I915_MAP_WC);
+			if (IS_ERR(ptr)) {
+				err = PTR_ERR(ptr);
+				goto err;
+			}
+			ptr = page_pack_bits(ptr, 1);
+		}
+
 		if (ptr == NULL) {
 			err = -ENOMEM;
 			goto err;
 		}
 
 		if (unlikely(cmpxchg(&vma->iomap, NULL, ptr))) {
-			io_mapping_unmap(ptr);
+			if (page_unmask_bits(ptr))
+				__i915_gem_object_release_map(vma->obj);
+			else
+				io_mapping_unmap(ptr);
 			ptr = vma->iomap;
 		}
 	}
@@ -596,7 +603,7 @@ void __iomem *i915_vma_pin_iomap(struct i915_vma *vma)
 	i915_vma_set_ggtt_write(vma);
 
 	/* NB Access through the GTT requires the device to be awake. */
-	return ptr;
+	return page_mask_bits(ptr);
 
 err_unpin:
 	__i915_vma_unpin(vma);
@@ -614,6 +621,8 @@ void i915_vma_unpin_iomap(struct i915_vma *vma)
 {
 	GEM_BUG_ON(vma->iomap == NULL);
 
+	/* XXX We keep the mapping until __i915_vma_unbind()/evict() */
+
 	i915_vma_flush_writes(vma);
 
 	i915_vma_unpin_fence(vma);
@@ -1761,7 +1770,10 @@ static void __i915_vma_iounmap(struct i915_vma *vma)
 	if (vma->iomap == NULL)
 		return;
 
-	io_mapping_unmap(vma->iomap);
+	if (page_unmask_bits(vma->iomap))
+		__i915_gem_object_release_map(vma->obj);
+	else
+		io_mapping_unmap(vma->iomap);
 	vma->iomap = NULL;
 }
 
-- 
2.25.1

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/2] drm/i915/display: Add smem fallback allocation for dpt

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/display: Add smem fallback allocation for dpt
URL   : https://patchwork.freedesktop.org/series/103678/
State : warning

== Summary ==

Error: dim checkpatch failed
cb0cd39c87aa drm/i915/display: Add smem fallback allocation for dpt
5fc338ca8432 drm/i915: Fix i915_vma_pin_iomap()
-:44: CHECK:BRACES: Unbalanced braces around else statement
#44: FILE: drivers/gpu/drm/i915/i915_vma.c:572:
+		else {

-:47: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#47: FILE: drivers/gpu/drm/i915/i915_vma.c:575:
+				i915_gem_object_pin_map_unlocked(vma->obj,
+								I915_MAP_WC);

total: 0 errors, 0 warnings, 2 checks, 73 lines checked

[Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915/display: Add smem fallback allocation for dpt

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 2136 bytes --]

== Series Details ==

Series: series starting with [1/2] drm/i915/display: Add smem fallback allocation for dpt
URL   : https://patchwork.freedesktop.org/series/103678/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_11617 -> Patchwork_103678v1
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103678v1/index.html

Participating hosts (45 -> 34)
------------------------------

  Additional (1): fi-snb-2520m 
  Missing    (12): fi-bxt-dsi bat-adls-5 bat-dg1-6 bat-dg2-8 bat-adlm-1 bat-dg2-9 fi-bsw-cyan bat-adlp-6 bat-adln-1 bat-rpls-2 bat-jsl-2 bat-jsl-1 

Known issues
------------

  Here are the changes found in Patchwork_103678v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_lmem_swapping@basic:
    - fi-snb-2520m:       NOTRUN -> [SKIP][1] ([fdo#109271]) +24 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103678v1/fi-snb-2520m/igt@gem_lmem_swapping@basic.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-snb-2520m:       NOTRUN -> [SKIP][2] ([fdo#109271] / [fdo#111827]) +8 similar issues
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103678v1/fi-snb-2520m/igt@kms_chamelium@hdmi-hpd-fast.html

  
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827


Build changes
-------------

  * Linux: CI_DRM_11617 -> Patchwork_103678v1

  CI-20190529: 20190529
  CI_DRM_11617: d96cea3d7ffb524248fcc8db433c579cf262eaea @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6468: cffa5fffe9acddf49565b4caeeb5e3355ff2ea44 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_103678v1: d96cea3d7ffb524248fcc8db433c579cf262eaea @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

917898e4eee6 drm/i915: Fix i915_vma_pin_iomap()
07c3d8bd88b1 drm/i915/display: Add smem fallback allocation for dpt

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103678v1/index.html

[-- Attachment #2: Type: text/html, Size: 2841 bytes --]

[Intel-gfx] ✓ Fi.CI.IGT: success for series starting with [1/2] drm/i915/display: Add smem fallback allocation for dpt

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 3174 bytes --]

== Series Details ==

Series: series starting with [1/2] drm/i915/display: Add smem fallback allocation for dpt
URL   : https://patchwork.freedesktop.org/series/103678/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_11617_full -> Patchwork_103678v1_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103678v1/index.html

Participating hosts (11 -> 7)
------------------------------

  Missing    (4): pig-skl-6260u pig-kbl-iris shard-tglu pig-glk-j5005 

Known issues
------------

  Here are the changes found in Patchwork_103678v1_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_flush@basic-batch-kernel-default-uc:
    - shard-snb:          [PASS][1] -> [SKIP][2] ([fdo#109271])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11617/shard-snb7/igt@gem_exec_flush@basic-batch-kernel-default-uc.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103678v1/shard-snb6/igt@gem_exec_flush@basic-batch-kernel-default-uc.html

  * igt@kms_chamelium@dp-hpd-storm:
    - shard-snb:          NOTRUN -> [SKIP][3] ([fdo#109271] / [fdo#111827])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103678v1/shard-snb7/igt@kms_chamelium@dp-hpd-storm.html

  * igt@kms_hdmi_inject@inject-4k:
    - shard-snb:          NOTRUN -> [SKIP][4] ([fdo#109271]) +8 similar issues
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103678v1/shard-snb7/igt@kms_hdmi_inject@inject-4k.html

  
#### Possible fixes ####

  * igt@gem_exec_flush@basic-wb-prw-default:
    - shard-snb:          [SKIP][5] ([fdo#109271]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11617/shard-snb6/igt@gem_exec_flush@basic-wb-prw-default.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103678v1/shard-snb5/igt@gem_exec_flush@basic-wb-prw-default.html

  * igt@i915_selftest@live@hangcheck:
    - shard-snb:          [INCOMPLETE][7] ([i915#3921]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11617/shard-snb5/igt@i915_selftest@live@hangcheck.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103678v1/shard-snb7/igt@i915_selftest@live@hangcheck.html

  
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#3921]: https://gitlab.freedesktop.org/drm/intel/issues/3921


Build changes
-------------

  * Linux: CI_DRM_11617 -> Patchwork_103678v1
  * Piglit: piglit_4509 -> None

  CI-20190529: 20190529
  CI_DRM_11617: d96cea3d7ffb524248fcc8db433c579cf262eaea @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6468: cffa5fffe9acddf49565b4caeeb5e3355ff2ea44 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_103678v1: d96cea3d7ffb524248fcc8db433c579cf262eaea @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_103678v1/index.html

[-- Attachment #2: Type: text/html, Size: 4099 bytes --]

Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Add smem fallback allocation for dpt

[Matthew Auld]

On Fri, 6 May 2022 at 14:11, Juha-Pekka Heikkila
<juhapekka.heikkila@gmail.com> wrote:
>
> Add fallback smem allocation for dpt if stolen memory allocation failed.
>
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dpt.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dpt.c b/drivers/gpu/drm/i915/display/intel_dpt.c
> index fb0e7e79e0cd..10008699656e 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpt.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpt.c
> @@ -10,6 +10,7 @@
>  #include "intel_display_types.h"
>  #include "intel_dpt.h"
>  #include "intel_fb.h"
> +#include "gem/i915_gem_internal.h"

Nit: Keep these ordered.

>
>  struct i915_dpt {
>         struct i915_address_space vm;
> @@ -128,6 +129,10 @@ struct i915_vma *intel_dpt_pin(struct i915_address_space *vm)
>         void __iomem *iomem;
>         struct i915_gem_ww_ctx ww;
>         int err;
> +       u64 pin_flags = 0;

Nit: Christmas tree-ish. Move this above the err.

> +
> +       if (!i915_gem_object_is_lmem(dpt->obj))
> +               pin_flags |= PIN_MAPPABLE;

If we do this then we don't need the second patch ;)

I guess the second patch was meant to make this is_stolen? Maybe just
move the second patch to be the first in the series?

>
>         wakeref = intel_runtime_pm_get(&i915->runtime_pm);
>         atomic_inc(&i915->gpu_error.pending_fb_pin);
> @@ -138,7 +143,7 @@ struct i915_vma *intel_dpt_pin(struct i915_address_space *vm)
>                         continue;
>
>                 vma = i915_gem_object_ggtt_pin_ww(dpt->obj, &ww, NULL, 0, 4096,
> -                                                 HAS_LMEM(i915) ? 0 : PIN_MAPPABLE);
> +                                                 pin_flags);
>                 if (IS_ERR(vma)) {
>                         err = PTR_ERR(vma);
>                         continue;
> @@ -248,10 +253,13 @@ intel_dpt_create(struct intel_framebuffer *fb)
>
>         size = round_up(size * sizeof(gen8_pte_t), I915_GTT_PAGE_SIZE);
>
> -       if (HAS_LMEM(i915))
> -               dpt_obj = i915_gem_object_create_lmem(i915, size, I915_BO_ALLOC_CONTIGUOUS);
> -       else
> +       dpt_obj = i915_gem_object_create_lmem(i915, size, I915_BO_ALLOC_CONTIGUOUS);
> +       if (IS_ERR(dpt_obj) && i915_ggtt_has_aperture(to_gt(i915)->ggtt))
>                 dpt_obj = i915_gem_object_create_stolen(i915, size);
> +       if (IS_ERR(dpt_obj) && !HAS_LMEM(i915)) {
> +               drm_dbg_kms(&i915->drm, "Allocating dpt from smem\n");

Hopefully this is not too noisy?

With the s/is_lmem/is_stolen/, or however you want to deal with that,
Reviewed-by: Matthew Auld <matthew.auld@intel.com>

> +               dpt_obj = i915_gem_object_create_internal(i915, size);
> +       }
>         if (IS_ERR(dpt_obj))
>                 return ERR_CAST(dpt_obj);
>
> --
> 2.25.1
>

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Fix i915_vma_pin_iomap()

[Matthew Auld]

On Fri, 6 May 2022 at 14:11, Juha-Pekka Heikkila
<juhapekka.heikkila@gmail.com> wrote:
>
> From: CQ Tang <cq.tang@intel.com>
>
> Display might allocate a smem object and call
> i915_vma_pin_iomap(), the existing code will fail.
>
> This fix was suggested by Chris P Wilson, that we pin
> the smem with i915_gem_object_pin_map_unlocked().
>
> Signed-off-by: CQ Tang <cq.tang@intel.com>
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> Cc: Chris Wilson <chris.p.wilson@intel.com>
> Cc: Jari Tahvanainen <jari.tahvanainen@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_vma.c | 34 ++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index 162e8d83691b..8ce016ef3dba 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -550,13 +550,6 @@ void __iomem *i915_vma_pin_iomap(struct i915_vma *vma)
>         if (WARN_ON_ONCE(vma->obj->flags & I915_BO_ALLOC_GPU_ONLY))
>                 return IO_ERR_PTR(-EINVAL);
>
> -       if (!i915_gem_object_is_lmem(vma->obj)) {
> -               if (GEM_WARN_ON(!i915_vma_is_map_and_fenceable(vma))) {
> -                       err = -ENODEV;
> -                       goto err;
> -               }
> -       }
> -
>         GEM_BUG_ON(!i915_vma_is_ggtt(vma));
>         GEM_BUG_ON(!i915_vma_is_bound(vma, I915_VMA_GLOBAL_BIND));
>         GEM_BUG_ON(i915_vma_verify_bind_complete(vma));
> @@ -572,17 +565,31 @@ void __iomem *i915_vma_pin_iomap(struct i915_vma *vma)
>                 if (i915_gem_object_is_lmem(vma->obj))
>                         ptr = i915_gem_object_lmem_io_map(vma->obj, 0,
>                                                           vma->obj->base.size);
> -               else
> +               else if (i915_vma_is_map_and_fenceable(vma))
>                         ptr = io_mapping_map_wc(&i915_vm_to_ggtt(vma->vm)->iomap,
>                                                 vma->node.start,
>                                                 vma->node.size);
> +               else {
> +                       ptr = (void __iomem *)
> +                               i915_gem_object_pin_map_unlocked(vma->obj,
> +                                                               I915_MAP_WC);

Note that with this patch we could now also get rid of lmem_io_map()
and just use this path instead. We just need to ensure we always have
the object lock held when calling pin_iomap, and drop the _unlocked
here, or maybe add an pin_iomap_unlocked if that is easier. If we are
always holding the object lock, we can then also maybe drop the
lockless tricks below...

> +                       if (IS_ERR(ptr)) {
> +                               err = PTR_ERR(ptr);
> +                               goto err;
> +                       }
> +                       ptr = page_pack_bits(ptr, 1);

We should try to avoid pointer packing, but I guess here this is just
re-using the mm.mapping stuff.

> +               }
> +
>                 if (ptr == NULL) {
>                         err = -ENOMEM;
>                         goto err;
>                 }
>
>                 if (unlikely(cmpxchg(&vma->iomap, NULL, ptr))) {
> -                       io_mapping_unmap(ptr);
> +                       if (page_unmask_bits(ptr))
> +                               __i915_gem_object_release_map(vma->obj);
> +                       else
> +                               io_mapping_unmap(ptr);
>                         ptr = vma->iomap;
>                 }
>         }
> @@ -596,7 +603,7 @@ void __iomem *i915_vma_pin_iomap(struct i915_vma *vma)
>         i915_vma_set_ggtt_write(vma);
>
>         /* NB Access through the GTT requires the device to be awake. */
> -       return ptr;
> +       return page_mask_bits(ptr);
>
>  err_unpin:
>         __i915_vma_unpin(vma);
> @@ -614,6 +621,8 @@ void i915_vma_unpin_iomap(struct i915_vma *vma)
>  {
>         GEM_BUG_ON(vma->iomap == NULL);
>
> +       /* XXX We keep the mapping until __i915_vma_unbind()/evict() */
> +
>         i915_vma_flush_writes(vma);
>
>         i915_vma_unpin_fence(vma);
> @@ -1761,7 +1770,10 @@ static void __i915_vma_iounmap(struct i915_vma *vma)
>         if (vma->iomap == NULL)
>                 return;
>
> -       io_mapping_unmap(vma->iomap);
> +       if (page_unmask_bits(vma->iomap))
> +               __i915_gem_object_release_map(vma->obj);
> +       else
> +               io_mapping_unmap(vma->iomap);
>         vma->iomap = NULL;

I don't suppose you want to type a patch that also moves the
__i915_vma_iounmap() in vma_evict out from under the
is_map_and_fenceable check, since we are currently leaking that lmem
mapping, and now also that pin_map? I could have sworn that internal
fixed that somewhere...

With that "leak" fixed,
Reviewed-by: Matthew Auld <matthew.auld@intel.com>

>  }





>
> --
> 2.25.1
>

Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Add smem fallback allocation for dpt

[Juha-Pekka Heikkilä]

Matthew Auld kirjoitti 11.5.2022 klo 13.41:
> On Fri, 6 May 2022 at 14:11, Juha-Pekka Heikkila
> <juhapekka.heikkila@gmail.com> wrote:
>>
>> Add fallback smem allocation for dpt if stolen memory allocation failed.
>>
>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_dpt.c | 16 ++++++++++++----
>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dpt.c b/drivers/gpu/drm/i915/display/intel_dpt.c
>> index fb0e7e79e0cd..10008699656e 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dpt.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dpt.c
>> @@ -10,6 +10,7 @@
>>   #include "intel_display_types.h"
>>   #include "intel_dpt.h"
>>   #include "intel_fb.h"
>> +#include "gem/i915_gem_internal.h"
> 
> Nit: Keep these ordered.
> 
>>
>>   struct i915_dpt {
>>          struct i915_address_space vm;
>> @@ -128,6 +129,10 @@ struct i915_vma *intel_dpt_pin(struct i915_address_space *vm)
>>          void __iomem *iomem;
>>          struct i915_gem_ww_ctx ww;
>>          int err;
>> +       u64 pin_flags = 0;
> 
> Nit: Christmas tree-ish. Move this above the err.
> 
>> +
>> +       if (!i915_gem_object_is_lmem(dpt->obj))
>> +               pin_flags |= PIN_MAPPABLE;
> 
> If we do this then we don't need the second patch ;)
> 
> I guess the second patch was meant to make this is_stolen? Maybe just
> move the second patch to be the first in the series?
> 

Hi Matthew, thanks for the comments. I think I'm still missing some 
essential part. Without marking PIN_MAPPABLE when !lmem I was hitting 
WARN_ON() in gem code when doing this pinning. Weird thing with it was I 
got everything working with y-tile but x-tile was 100% fail. I'll need 
to repro those results and see why I got that. There was recently fixed 
regression on igt side which may have affected my results but I'll 
probably anyway need to have another round for these patches including 
fixes to those parts you pointed out.

/Juha-Pekka

>>
>>          wakeref = intel_runtime_pm_get(&i915->runtime_pm);
>>          atomic_inc(&i915->gpu_error.pending_fb_pin);
>> @@ -138,7 +143,7 @@ struct i915_vma *intel_dpt_pin(struct i915_address_space *vm)
>>                          continue;
>>
>>                  vma = i915_gem_object_ggtt_pin_ww(dpt->obj, &ww, NULL, 0, 4096,
>> -                                                 HAS_LMEM(i915) ? 0 : PIN_MAPPABLE);
>> +                                                 pin_flags);
>>                  if (IS_ERR(vma)) {
>>                          err = PTR_ERR(vma);
>>                          continue;
>> @@ -248,10 +253,13 @@ intel_dpt_create(struct intel_framebuffer *fb)
>>
>>          size = round_up(size * sizeof(gen8_pte_t), I915_GTT_PAGE_SIZE);
>>
>> -       if (HAS_LMEM(i915))
>> -               dpt_obj = i915_gem_object_create_lmem(i915, size, I915_BO_ALLOC_CONTIGUOUS);
>> -       else
>> +       dpt_obj = i915_gem_object_create_lmem(i915, size, I915_BO_ALLOC_CONTIGUOUS);
>> +       if (IS_ERR(dpt_obj) && i915_ggtt_has_aperture(to_gt(i915)->ggtt))
>>                  dpt_obj = i915_gem_object_create_stolen(i915, size);
>> +       if (IS_ERR(dpt_obj) && !HAS_LMEM(i915)) {
>> +               drm_dbg_kms(&i915->drm, "Allocating dpt from smem\n");
> 
> Hopefully this is not too noisy?
> 
> With the s/is_lmem/is_stolen/, or however you want to deal with that,
> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
> 
>> +               dpt_obj = i915_gem_object_create_internal(i915, size);
>> +       }
>>          if (IS_ERR(dpt_obj))
>>                  return ERR_CAST(dpt_obj);
>>
>> --
>> 2.25.1
>>

Re: [Intel-gfx] [PATCH 1/2] drm/i915/display: Add smem fallback allocation for dpt

[Matthew Auld]

On Fri, 20 May 2022 at 12:23, Juha-Pekka Heikkilä
<juhapekka.heikkila@gmail.com> wrote:
>
>
>
> Matthew Auld kirjoitti 11.5.2022 klo 13.41:
> > On Fri, 6 May 2022 at 14:11, Juha-Pekka Heikkila
> > <juhapekka.heikkila@gmail.com> wrote:
> >>
> >> Add fallback smem allocation for dpt if stolen memory allocation failed.
> >>
> >> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> >> ---
> >>   drivers/gpu/drm/i915/display/intel_dpt.c | 16 ++++++++++++----
> >>   1 file changed, 12 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_dpt.c b/drivers/gpu/drm/i915/display/intel_dpt.c
> >> index fb0e7e79e0cd..10008699656e 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_dpt.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_dpt.c
> >> @@ -10,6 +10,7 @@
> >>   #include "intel_display_types.h"
> >>   #include "intel_dpt.h"
> >>   #include "intel_fb.h"
> >> +#include "gem/i915_gem_internal.h"
> >
> > Nit: Keep these ordered.
> >
> >>
> >>   struct i915_dpt {
> >>          struct i915_address_space vm;
> >> @@ -128,6 +129,10 @@ struct i915_vma *intel_dpt_pin(struct i915_address_space *vm)
> >>          void __iomem *iomem;
> >>          struct i915_gem_ww_ctx ww;
> >>          int err;
> >> +       u64 pin_flags = 0;
> >
> > Nit: Christmas tree-ish. Move this above the err.
> >
> >> +
> >> +       if (!i915_gem_object_is_lmem(dpt->obj))
> >> +               pin_flags |= PIN_MAPPABLE;
> >
> > If we do this then we don't need the second patch ;)
> >
> > I guess the second patch was meant to make this is_stolen? Maybe just
> > move the second patch to be the first in the series?
> >
>
> Hi Matthew, thanks for the comments. I think I'm still missing some
> essential part. Without marking PIN_MAPPABLE when !lmem I was hitting
> WARN_ON() in gem code when doing this pinning.

What was the WARN_ON? Got a paste?