Hi,
On 05-04-18 15:26, Daniel Vetter wrote:
On Thu, Apr 5, 2018 at 1:47 PM, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
Hi,
On 05-04-18 09:14, Daniel Vetter wrote:
On Fri, Mar 30, 2018 at 02:27:15PM +0200, Hans de Goede wrote:
Before this commit the WaSkipStolenMemoryFirstPage workaround code was
skipping the first 4k by passing 4096 as start of the address range
passed
to drm_mm_init(). This means that calling drm_mm_reserve_node() to try
and
reserve the firmware framebuffer so that we can inherit it would always
fail, as the firmware framebuffer starts at address 0.
Commit d43537610470 ("drm/i915: skip the first 4k of stolen memory on
everything >= gen8") says in its commit message: "This is confirmed to
fix
Skylake screen flickering issues (probably caused by the fact that we
initialized a ring in the first page of stolen, but I didn't 100% confirm
this theory)."
Which suggests that it is safe to use the first page for a linear
framebuffer as the firmware is doing.
This commit always passes 0 as start to drm_mm_init() and works around
WaSkipStolenMemoryFirstPage in i915_gem_stolen_insert_node_in_range()
by insuring the start address passed by to drm_mm_insert_node_in_range()
is always 4k or more. All entry points to i915_gem_stolen.c go through
i915_gem_stolen_insert_node_in_range(), so that any newly allocated
objects such as ring-buffers will not be allocated in the first 4k.
The one exception is i915_gem_object_create_stolen_for_preallocated()
which directly calls drm_mm_reserve_node() which now will be able to
use the first 4k.
This fixes the i915 driver no longer being able to inherit the firmware
framebuffer on gen8+, which fixes the video output changing from the
vendor logo to a black screen as soon as the i915 driver is loaded
(on systems without fbcon).
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
I think this is worth a shot. The only explanation I can think of why the
GOP could get away with this and still follow the w/a is if it doesn't
have a 1:1 mapping between GGTT and stolen.
My guess is that the GOP does not care about the w/a because the w/a
states that the command-streamers sometimes do a spurious flush (write)
to the first page, and the command-streamers are not used until much
later, so the GOP is probably ignoring the w/a all together.
At least that is my theory.
Atm we do not know whether the GOP actually implements the wa or not.
That it doesn't care is just a conjecture, but we have no proof. On
previous platforms the 1:1 mapping did hold, but there's no
fundamental reason why it sitll does.
Right.
Atm we hardcode that
assumption in intel_alloc_initial_plane_obj by passing the base_aligned as
both the stolen_offset and the gtt_offset (but it's only the gtt_offset
really). And since we're not re-writing the ptes it's not noticeable.
I think to decide whether this is the right approach we should
double-check whether that 1:1 assumption really holds true: Either read
back the ggtt ptes and check their addresses (but iirc on some platforms
their write-only, readback doesn't work), or we also rewrite the ptes
again for preallocated stuff, like when binding a normal object into the
gtt. If either of these approaches confirms that those affected gen8+
machines still use the 1:1 mapping, then I'm happy to put my r-b on this
patch. If not, well then we at least know what to fix: We need to read out
the real stolen_offset, instead of making assumptions.
I'm happy to give this a try on one or more affected machines, I can e.g.
try this on both a skylake desktop and a cherry-trail based tablet.
But I'm clueless about the whole PTE stuff, so I'm going to need someone
to provide me with a patch following either approach. If readback of the
PTEs works on skylake / cherrytrail I guess that would be the best way.
Note to test this I'm currently booting the kernel with the machine's
UEFI vendor logo still being displayed when efifb kicks in. I've added:
"fbcon=vc:2-62" to the kernel commandline to make fbcon not bind to
VC0 / VC1, so that the framebuffer contents stays intact, combined with
the patch we are discussing now, this makes the vendor logo stay on
the screen all the way till GDM loads, which looks rather nice actually :)
And on shutdown we fall back to the original framebuffer and the vendor
logo is back again. I cannot see any corruption in the display at either
boot or shutdown time.
It shouldn't matter whether efifb takes over or not, it's still the
GOP's framebuffer we're taking over. Different content for sure, logo
is gone, but we only care about which pages we're using.
Wrt the code:
- (Re)binding happens by calling i915_vma_bind, if you put a call to
that at the end of the stolen_preallocated functions you should get
that. Ofc needs the logo still there so you can see it jump/get
mangled.
- GGTT PTEs for gen8+ are written through e.g. gen8_ggttt_insert_page
or gen8_ggtt_insert_entries (which takes the vma). Since we only care
about the first entry a readq(dev_priv->ggtt->gsm) is all we really
need. If it's all 0 then it's not working (since at least
_PAGE_PRESENT should be set, plus the physical address of a page in
stolen memory).
I can do some patches for each of those, but a bit swamped. Also no
gen8 handy for testing I think to make sure it works, so would take a
few weeks probably.
I was a bit swamped with other stuff myself too, but I'm back to working
on this now. Below is the debug patch I came up with based on what you
wrote above.
This results in the following debug output:
[ 1.651141] first ggtt entry before bind: 0x0000000078c00001
[ 1.651151] i915_vma_bind ret 0
[ 1.651152] first ggtt entry after bind: 0x0000000078c00083
And "sudo cat /proc/iomem | grep Stolen" gives:
78c00000-88bfffff : Graphics Stolen Memory
I see no visual changes with this patch (BIOS vendor logo still
stays in place when using fbcon=vc:2-62 to make fbcon not bind
on boot and keep the vendor logo intact).
The address of the first ggtt entry matches with the start of stolen
and the i915_vma_bind call only changes the first gtt entry's flags,
or-ing in _PAGE_RW (BIT(1)) and PPAT_CACHED (BIT(7)), which perfectly
matches what we would expect based on gen8_pte_encode()'s behavior.
So it seems that the GOP indeed does NOT implement the wa and the i915's
code assuming a linear mapping at the start of stolen for the BIOS fb
still holds true.
The debug output is from my Skylake based machine which uses an
Asrock B150M Pro4S/D3 mainboard.
I've also tested this on a Cherry Trail based device (a GPD Win)
with identical results (the flags are 0x1b after the vma_bind
on CHT, which matches with I915_CACHE_NONE).
Regards,
Hans
From 400f2b0d722236999a5706a1f2fa0bb70bf9c73f Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@xxxxxxxxxx>
Date: Thu, 19 Apr 2018 14:44:43 +0200
Subject: [PATCH resend] i915: i915_vma_bind for pre-allocated stolen test /
debug
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
drivers/gpu/drm/i915/i915_gem_stolen.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 152487d91197..7db03f7d2c51 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -648,6 +648,13 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_i915_private *dev_priv
obj->bind_count++;
spin_unlock(&dev_priv->mm.obj_lock);
+ pr_err("first ggtt entry before bind: 0x%016llx\n", readq(dev_priv->ggtt.gsm));
+ ret = i915_vma_bind(vma,
+ HAS_LLC(dev_priv) ? I915_CACHE_LLC : I915_CACHE_NONE,
+ PIN_UPDATE);
+ pr_err("i915_vma_bind ret %d\n", ret);
+ pr_err("first ggtt entry after bind: 0x%016llx\n", readq(dev_priv->ggtt.gsm));
+
return obj;
err_pages:
--
2.17.0
Cheers, Daniel
Cheers, Daniel
---
drivers/gpu/drm/i915/i915_gem_stolen.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c
b/drivers/gpu/drm/i915/i915_gem_stolen.c
index af915d041281..ad949cc30928 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -51,6 +51,10 @@ int i915_gem_stolen_insert_node_in_range(struct
drm_i915_private *dev_priv,
if (!drm_mm_initialized(&dev_priv->mm.stolen))
return -ENODEV;
+ /* WaSkipStolenMemoryFirstPage:bdw+ */
+ if (INTEL_GEN(dev_priv) >= 8 && start < 4096)
+ start = 4096;
+
mutex_lock(&dev_priv->mm.stolen_lock);
ret = drm_mm_insert_node_in_range(&dev_priv->mm.stolen, node,
size, alignment, 0,
@@ -343,7 +347,6 @@ int i915_gem_init_stolen(struct drm_i915_private
*dev_priv)
{
resource_size_t reserved_base, stolen_top;
resource_size_t reserved_total, reserved_size;
- resource_size_t stolen_usable_start;
mutex_init(&dev_priv->mm.stolen_lock);
@@ -435,17 +438,11 @@ int i915_gem_init_stolen(struct drm_i915_private
*dev_priv)
(u64)resource_size(&dev_priv->dsm) >> 10,
((u64)resource_size(&dev_priv->dsm) -
reserved_total) >> 10);
- stolen_usable_start = 0;
- /* WaSkipStolenMemoryFirstPage:bdw+ */
- if (INTEL_GEN(dev_priv) >= 8)
- stolen_usable_start = 4096;
-
dev_priv->stolen_usable_size =
- resource_size(&dev_priv->dsm) - reserved_total -
stolen_usable_start;
+ resource_size(&dev_priv->dsm) - reserved_total;
/* Basic memrange allocator for stolen space. */
- drm_mm_init(&dev_priv->mm.stolen, stolen_usable_start,
- dev_priv->stolen_usable_size);
+ drm_mm_init(&dev_priv->mm.stolen, 0,
dev_priv->stolen_usable_size);
return 0;
}
--
2.17.0.rc2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
