From patchwork Thu Apr 19 13:43:19 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hans de Goede X-Patchwork-Id: 10350327 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 85DDB6023A for ; Thu, 19 Apr 2018 13:43:27 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 7636128A09 for ; Thu, 19 Apr 2018 13:43:27 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 6AA5C28A0E; Thu, 19 Apr 2018 13:43:27 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=2.0 tests=BAYES_00, MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 90EE828A09 for ; Thu, 19 Apr 2018 13:43:26 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id BCF9A6E078; Thu, 19 Apr 2018 13:43:24 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mail-wr0-f193.google.com (mail-wr0-f193.google.com [209.85.128.193]) by gabe.freedesktop.org (Postfix) with ESMTPS id 1B96D6E078 for ; Thu, 19 Apr 2018 13:43:23 +0000 (UTC) Received: by mail-wr0-f193.google.com with SMTP id v24-v6so14187094wra.8 for ; Thu, 19 Apr 2018 06:43:23 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=cI4FMVztkTYmpxtM8lv1oKGe51wwVXUtyu9MvYdvHio=; b=FGXp9J5F7IfGZS4Ao4xAyYMUhQn81vFuBn3qexAecWZvQl7mnX5Y5mK/wqGS4TM4AD YnU3dcQ9wm+N4mAbQ1s9gG71LIXeRdIApbMJPQ5f3wVBn0psJorfsfraqNLBIq//Sp2d MNlZBlx3tDBTze9BBk57TAWShXb3u9FzhZass04UQJlOybi99PKu4wloPEf7F3Wjq1XD H+8Y3NDg9PJbNURe8yMe8voRcM7FjPCDQPEtBpW/w13orM7d65XevCOTgW1I+IvDft0J wnFBHnzyn+IwQ5cnT1w4+JzIy/gQ9FM1SnV/dnYSkKY/J+ZopBNOz99Atd3B7o591rD0 JJ7w== X-Gm-Message-State: ALQs6tCErDL9DzMPWoCUq1/vSs1ydScM6jM/1ge8sSKqhv/vtfG467nC HH6HP9/duvujp4ksLodfLHtJUw== X-Google-Smtp-Source: AIpwx48M9jRRIcK8cl9rXF2Fi0mIIoY3NK/XWXgexuDoA2dtTpOgXa4UvY9/nXKXL2HlFryjBEnqgA== X-Received: by 10.80.168.161 with SMTP id k30mr8864637edc.63.1524145401374; Thu, 19 Apr 2018 06:43:21 -0700 (PDT) Received: from shalem.localdomain (546A5441.cm-12-3b.dynamic.ziggo.nl. [84.106.84.65]) by smtp.gmail.com with ESMTPSA id g55sm2490715eda.15.2018.04.19.06.43.20 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 19 Apr 2018 06:43:20 -0700 (PDT) To: Daniel Vetter References: <20180330122715.18209-1-hdegoede@redhat.com> <20180405071434.GN3881@phenom.ffwll.local> <0c848841-2ca7-cc30-39f0-8c630ad19cef@redhat.com> From: Hans de Goede Message-ID: <0ae3166a-e27a-648f-edb6-0280fd087bb3@redhat.com> Date: Thu, 19 Apr 2018 15:43:19 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US Subject: Re: [Intel-gfx] [PATCH] drm/i915: Do NOT skip the first 4k of stolen memory for pre-allocated buffers X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Hans de Goede , intel-gfx , dri-devel , Rodrigo Vivi Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Virus-Scanned: ClamAV using ClamSMTP Hi, On 05-04-18 15:26, Daniel Vetter wrote: > On Thu, Apr 5, 2018 at 1:47 PM, Hans de Goede 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 >>> >>> >>> 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 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 Reviewed-by: Daniel Vetter --- 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: