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 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.
Regards,
Hans
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
