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

[Hans de Goede]

Hi,

On 20-04-18 09:27, Daniel Vetter wrote:

On Thu, Apr 19, 2018 at 03:43:19PM +0200, Hans de Goede wrote:

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 

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

[Daniel Vetter]

On Thu, Apr 19, 2018 at 03:43:19PM +0200, Hans de Goede wrote:
> 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 

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

[Hans de Goede]

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 

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

[Daniel Vetter]

On Sat, Apr 07, 2018 at 11:34:57AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 06-04-18 18:06, Ville Syrjälä wrote:
> > 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.
> > > > > > > > > 
> > > > > > > > 

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

[Hans de Goede]

Hi,

On 06-04-18 18:06, Ville Syrjälä wrote:

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 

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

[Ville Syrjälä]

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
>  

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

[Daniel Vetter]

On Thu, Apr 5, 2018 at 3:31 PM, Hans de Goede  wrote:
> 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.
>>
 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 

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

[Hans de Goede]

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.


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.


Right, I only mentioned this to explain that I'm not seeing any
(visible) corruption.


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. 

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

[Daniel Vetter]

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.

>> 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 

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

[Hans de Goede]

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 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(_priv->mm.stolen))
return -ENODEV;
  
+	/* WaSkipStolenMemoryFirstPage:bdw+ */

+   if (INTEL_GEN(dev_priv) >= 8 && start < 4096)
+   start = 4096;
+
mutex_lock(_priv->mm.stolen_lock);
ret = drm_mm_insert_node_in_range(_priv->mm.stolen, node,
  size, alignment, 0,
@@ -343,7 +347,6 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
  {
  

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

[Hans de Goede]

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.), 

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

[Daniel Vetter]

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. 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(_priv->mm.stolen))
>   return -ENODEV;
>  
> + /* WaSkipStolenMemoryFirstPage:bdw+ */
> + if (INTEL_GEN(dev_priv) >= 8 && start < 4096)
> + start = 4096;
> +
>   mutex_lock(_priv->mm.stolen_lock);
>   ret = drm_mm_insert_node_in_range(_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(_priv->mm.stolen_lock);
>  
> @@ -435,17 +438,11 @@ int i915_gem_init_stolen(struct drm_i915_private 
> *dev_priv)
>(u64)resource_size(_priv->dsm) >> 10,
>((u64)resource_size(_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(_priv->dsm) - reserved_total - 
> stolen_usable_start;
> + resource_size(_priv->dsm) - reserved_total;
>  
>   /* Basic memrange allocator for stolen space. */
> - drm_mm_init(_priv->mm.stolen, stolen_usable_start,
> - dev_priv->stolen_usable_size);
> + drm_mm_init(_priv->mm.stolen, 0, dev_priv->stolen_usable_size);
>  
>   return 0;
>  }
> -- 
> 2.17.0.rc2
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> 

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

[Ville Syrjälä]

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 

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

[Hans de Goede]

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 

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

[Rodrigo Vivi]

On Wed, Apr 04, 2018 at 06:50:16PM +0300, 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 

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

[Ville Syrjälä]

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:

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

[Hans de Goede]

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 

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

[Hans de Goede]

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.

Regards,

Hans



___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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...
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Hans de Goede]

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.

Regards,

Hans




___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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.)
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx