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. 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.
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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
