[Intel-gfx] [PATCH] drm/i915: Do NOT skip the first 4k of stolen memory for pre-allocated buffers

Fri Apr 6 16:06:11 UTC 2018

On Thu, Apr 05, 2018 at 01:37:31PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 04-04-18 22:49, Ville Syrjälä wrote:
> > On Wed, Apr 04, 2018 at 10:06:29PM +0200, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 04-04-18 17:50, Ville Syrjälä wrote:
> >>> On Fri, Mar 30, 2018 at 04:26:53PM +0200, Hans de Goede wrote:
> >>>> Hi,
> >>>>
> >>>> On 30-03-18 15:25, Hans de Goede wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On 30-03-18 14:44, Chris Wilson wrote:
> >>>>>> Quoting Hans de Goede (2018-03-30 13:37:40)
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> On 30-03-18 14:30, Chris Wilson wrote:
> >>>>>>>> Quoting Hans de Goede (2018-03-30 13:27:15)
> >>>>>>>>> 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).
> >>>>>>>>
> >>>>>>>> We've been told by the HW guys not to use the first page. (That's my
> >>>>>>>> understanding from every time this gets questioned.)
> >>>>>>>
> >>>>>>> Yet the GOP is happily using the first page. I think we may need to make
> >>>>>>> a difference here between the GPU not using the first page and the
> >>>>>>> display engine/pipeline not using the first page. Note that my patch
> >>>>>>> only influences the inheriting of the initial framebuffer as allocated
> >>>>>>> by the GOP. It does not influence any other allocations from the
> >>>>>>> reserved range, those will still all avoid the first page.
> >>>>>>>
> >>>>>>> Without this patch fastboot / flickerfree support is essentially broken
> >>>>>>> on any gen8+ hardware given that one of the goals of atomic is to be
> >>>>>>> able to do flickerfree transitions I think that this warrants a closer
> >>>>>>> look then just simply saying never use the first page.
> >>>>>>
> >>>>>> The concern is what else (i.e. nothing that we allocated ourselves) that
> >>>>>> may be in the first page...
> >>>>>
> >>>>> Given that the GOP has put its framebuffer there at least at boot there
> >>>>> is nothing there, otherwise it would show up on the display.
> >>>>>
> >>>>> We have a whole bunch of code to inherit the BIOS fb in intel_display.c
> >>>>> and AFAIK that code is there because this inheriting the BIOS fb is
> >>>>> deemed an important feature. So I'm not happy at all with the handwavy
> >>>>> best to not touch it answer I'm getting to this patch.
> >>>>>
> >>>>> Unless there are some clear answer from the hardware folks which specifically
> >>>>> say we cannot put a framebuffer there (and then why is the GOP doing it?)
> >>>>> then I believe that the best way forward here is to get various people to
> >>>>> test with this patch and the best way to do that is probably to put it
> >>>>> in next. Note I deliberately did not add a Cc stable.
> >>>>
> >>>> To elaborate on this, the excluding of the first 4k of the stolen memory
> >>>> region causes intel_alloc_initial_plane_obj() from intel_display.c to fail,
> >>>> which in turn causes intel_find_initial_plane_obj() to call
> >>>> intel_plane_disable_noatomic(intel_crtc, intel_plane); which temporarily
> >>>> completely turns off the display which leads to a very ugly flickering
> >>>> of the display at boot (as well as replacing the vendor logo with a
> >>>> black screen).
> >>>>
> >>>> I think we can all agree that this behavior is undesirable and even a
> >>>> regression in behavior caused by the fix to exclude the first 4k.
> >>>>
> >>>> Chris, if my patch is not an acceptable way to fix this, then how do you
> >>>> suggest that we fix this?
> >>>>
> >>>> Digging a bit deeper I found this:
> >>>>
> >>>> https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-kbl-vol16-workarounds.pdf
> >>>>
> >>>> Which says:
> >>>>
> >>>> "WaSkipStolenMemoryFirstPage:
> >>>>
> >>>> WA to skip the first page of stolen
> >>>> memory due to sporadic HW write on *CS Idle"
> >>>>
> >>>> And also about FBC:
> >>>>
> >>>> "First line of FBC getting corrupted when FBC
> >>>> compressed frame buffer offset is programmed to
> >>>> zero. Command streamers are doing flush writes to
> >>>> base of stolen.
> >>>> WA: New restriction to program FBC compressed
> >>>> frame buffer offset to at least 4KB."
> >>>>
> >>>> So using the first 4kB for the *framebuffer* as done by the GOP will
> >>>> not cause any major problems (freezes, hangs, etc.), and commit
> >>>> d43537610470 ("drm/i915: skip the first 4k of stolen memory on
> >>>> everything >= gen8") was correct in deducing that the problem was
> >>>> likely that some *vital* information was being stored i the first 4k
> >>>> and that go overwritten.
> >>>>
> >>>> But the contents of the (first lines of) the framebuffer may become
> >>>> corrupted once we actually start using the command-streamers, which
> >>>> is still very much not wanted.
> >>>>
> >>>> In practice Xorg or Wayland will likely have setup another framebuffer
> >>>> by the time the command-streamers will start to get used.
> >>>>
> >>>> Alternatively we could start with inheriting the BIOS framebuffer
> >>>> (as my patch allows) so that we don't get the flicker and then soon
> >>>> afterwards atomically transit to a new framebuffer (which should
> >>>> contain a copy of the BIOS fb contents) at a different location.
> >>>
> >>> What I suggested long ago was to copy just the first page and adjust the
> >>> sg list. But I'm not sure if our stolen gem code would be happy with an
> >>> sg list with two entries instead of one.
> >>
> >> But that would still require an atomic-modeset to install the new sg-list,
> >> right?
> > 
> > Perhaps not. Not sure if the pte update would be atomic enough to just
> > change it underneath the display engine without ill effects, and then
> > do the equivalent of a page flip to invalidate the TLBs.
> > 
> >> Then we might just as well simply alloc a new fb and copy the
> >> contents over, or are you worried that with say a 4k fb that takes too
> >> much time? FWIW I can see how the single memcpy this involves will take
> >> some time, but I don't take it will take so long as to be a problem.
> > 
> > Mainly just a question of keeping it in stolen.
> 
> Ah I see.
> 
> > Assuming we want to keep
> > things in stolen, which is a matter of some debate as FBC needs stolen
> > and people might not be happy if it's all taken up by fbdev.
> > 
> >>
> >> Anyways I could use some help with implementing either solution as I'm
> >> not familiar with the involved parts of the code. I will happily test
> >> a patch for this. Keep in mind that for this to work my original patch
> >> will also be necessary so that the initial takeover of the firmware
> >> fb will work.
> > 
> > I guess the trickiest part would be getting both the old and new
> > location of the page mapped in the ggtt at the same time. Sadly you're
> > not allowed to access stolen directly. So I suppose this part would
> > involve some fairly low level frobbing of the ggtt ptes and a
> > manual ioremap() of the matching ranges of the aperture.
> 
> Hmm, you're talking about what needs to be done to copy the contents here,
> right?

Yeah.

> I have a feeling we really should just try only my patch first, as
> mentioned before the worst thing which can happen is some corruption
> of the first lines of the display, which I agree is not good, but also
> not the end of the world.

One can hope :)

-- 
Ville Syrjälä
Intel OTC