On 22.3.2022 12.45, Matthew Auld wrote:
On Mon, 21 Mar 2022 at 18:36, Juha-Pekka Heikkila
<juhapekka.heikkila@xxxxxxxxx> wrote:
On 21.3.2022 14.29, Matthew Auld wrote:
On Fri, 18 Mar 2022 at 09:22, Juha-Pekka Heikkila
<juhapekka.heikkila@xxxxxxxxx> wrote:
On 17.3.2022 13.55, Matthew Auld wrote:
On Wed, 16 Mar 2022 at 22:23, Juha-Pekka Heikkila
<juhapekka.heikkila@xxxxxxxxx> wrote:
Add fallback smem allocation for dpt if stolen memory
allocation failed.
Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@xxxxxxxxx>
---
drivers/gpu/drm/i915/display/intel_dpt.c | 18 ++++++++++++++----
1 file changed, 14 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..c8b66433d4db 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: these should be kept sorted
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_stolen(dpt->obj))
+ pin_flags |= PIN_MAPPABLE; /* for i915_vma_pin_iomap(stolen) */
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,15 @@ 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, "fb: [FB:%d] Allocating dpt from smem\n",
+ fb->base.base.id);
+
+ dpt_obj = i915_gem_object_create_internal(i915, size);
Looks like we are missing some prerequisite patch to be able to
directly map such memory in vma_pin_iomap?
For these functions I'm more like a consumer, I was following
suggestions from Chris on this. Is there something extra that should be
considered in this regard when use it like this?
AFAICT this will trigger the WARN_ON() in vma_pin_iomap() if we
fallback to create_internal(), since the object is now not lmem and is
also not map_and_fenceable(i.e PIN_MAPPABLE).
This shouldn't affect case when dpt allocation from lmem failed, it is
expected to go to "return ERR_CAST(dpt_obj);" below these comments. On
situation when allocating lmem and stolen failed on next "if" I added
!HAS_LMEM(i915) to handle situation with lmem. Though, when I was
originally trying this patch without limiting lmem case I remember with
dg2 I got black screen but I don't remember seeing WARN_ON() in logs.
The other issue is that we need some way of CPU mapping this type of
object, like with calling i915_gem_object_pin_map() inside
vma_pin_iomap(). It looks like there is an internal patch that tries
to handle both issues, so I guess we need to also bring that patch
upstream as a prerequisite to this?
I have above in intel_dpt_pin(..) that "pin_flags |= PIN_MAPPABLE" when
handling stolen memory. I suspect patch you are referring to is this
same patch I wrote, here just adjusted for upstreaming. This patch was
earlier tried by Lucas and Manasi to be working with adlp and apparently
cases with virtual machine this make it possible to have tiled
framebuffers. Without this patch those special cases will get -e2big
when creating tiled fb and no stolen memory available.
When the GGTT pin eventually ends up returning some vma that is not
within the ggtt->mappable_end, then we will start hitting the above
issues, starting with the WARN_ON. If you use PIN_HIGH here for the
non-stolen case, it should highlight the issue more reliably I think.
You mean once there's no space left in stolen there would be WARN_ON()?
This is case which was earlier tested by Lucas and Manasi on adlp to be
working correctly, this was on top of drm-tip. Also on internal testing
you can see platforms taking this path reliably with no errors.
I'm not sure why use PIN_HIGH for non stolen case, my exposure to gem
related parts is limited hence I was following Chris's suggestion to put
zero flag for i915_gem_object_ggtt_pin_ww(..) when not using stolen.
/Juha-pekka
