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

Thu Apr 5 11:37:31 UTC 2018

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?

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.

Regards,

Hans