Created attachment 6346 [details] [review]
Patch to make the function not swap out pinned pixmaps

... And the patch

Created attachment 6347 [details] [review]
New patch

...Hmm, new patch needed.

SwapOut is only used by EnableDisableFBAccess, which has no choice but to kick
out everything. If you need to know when a pinned area gets kicked, pass the
save argument to exaOffscreenAlloc.

Yes, I noticed that, but in what cases are exaEnableDisableFBAccess really used? 

I was trying to allocate a big chunk from the EXA offscreen manager to use for
DRI, and thought it was eqiuivalent to reserving that part of memory before the
EXA memory manager was initialized. Shouldn't it be?

/Thomas

(In reply to comment #4)
> Yes, I noticed that, but in what cases are exaEnableDisableFBAccess really used?

E.g. when VT switching away from the server. Generally, all FB contents have to
be assumed lost after disabling and re-enabling FB access.

> I was trying to allocate a big chunk from the EXA offscreen manager to use for
> DRI, and thought it was eqiuivalent to reserving that part of memory before the
> EXA memory manager was initialized. Shouldn't it be?

If you can't or don't want to provide a save hook, just put it outside of EXA
offscreen memory. You might even be able to do that dynamically by disabling FB
access, adjusting the EXA offscreen memory range and re-enabling FB access.

(In reply to comment #5)
> (In reply to comment #4)
> > Yes, I noticed that, but in what cases are exaEnableDisableFBAccess really used?
> 
> E.g. when VT switching away from the server. Generally, all FB contents have to
> be assumed lost after disabling and re-enabling FB access.


OK. Marking this notabug, but this complicates things. So a driver that, for
example, reserves an FB area for DRI should really provide an xxxEnableDisable
function, and save the DRI memory inbetween disable and enable calls?

> 
> > I was trying to allocate a big chunk from the EXA offscreen manager to use for
> > DRI, and thought it was eqiuivalent to reserving that part of memory before the
> > EXA memory manager was initialized. Shouldn't it be?
> 
> If you can't or don't want to provide a save hook, just put it outside of EXA
> offscreen memory. You might even be able to do that dynamically by disabling FB
> access, adjusting the EXA offscreen memory range and re-enabling FB access.

OK, thanks.

/Thomas

Hmm, The behaviour is inconsistent with xaa, 
which actually only moves out its own memory areas, 
which I think is the most logical behavior.

 

OK, been doing some thinking, I think this needs to be reopened.

The good thing with pinned areas are that they are pinned. If they are not, they
are actually rather useless. There is no way, for example, to tell an XvMC dri
client that the surface offset has suddenly changed. Which makes me forced to do
a separate fb area for XvMC surfaces, or have it allocate only out of DRI space
which is a HUGE waste of resources.

If the content is lost, fine, the X server can always reload that, but the
framebuffer offset should not change. 

If we want to have general or user-supplied EXA allocators in the future, I
think EXA needs to do like XAA. Have a enableDisableFBAccess function that swaps
out _only_ the pixmaps managed by EXA and let the driver manage its own areas
from enterVT / leaveVT.

This is basically what the last patch does, unless EXA pins some pixmaps of its own.

(In reply to comment #8)
> The good thing with pinned areas are that they are pinned. If they are not, they
> are actually rather useless.

The difference to unpinned areas is that pinned areas won't be kicked out to
satisfy another allocation.

> There is no way, for example, to tell an XvMC dri client that the surface
offset has suddenly changed. Which makes me forced to do
> a separate fb area for XvMC surfaces, or have it allocate only out of DRI space
> which is a HUGE waste of resources.
> 
> If the content is lost, fine, the X server can always reload that, but the
> framebuffer offset should not change. 

Dynamically changing the EXA offscreen memory range as described in comment #5
might work for this, but the disabling and re-enabling of FB access might cause
the display to noticeably stop updating for a bit.

You could also enforce constant offsets by re-allocating the pinned areas in a
driver EnableDisableFBAccess wrapper.

> If we want to have general or user-supplied EXA allocators in the future, I
> think EXA needs to do like XAA. Have a enableDisableFBAccess function that swaps
> out _only_ the pixmaps managed by EXA and let the driver manage its own areas
> from enterVT / leaveVT.

Quite frankly, I wouldn't take 'XAA does it like this' as a justification for
anything. :) Sounds like an XAA bug to me if it just ignores the fact that all
of video RAM may be wiped between disabling and re-enabling FB access.

Anyway, I'm adding Eric Anholt to the CC list as I'm curious if he has any
thoughts on this.

(In reply to comment #9)
> (In reply to comment #8)
> > The good thing with pinned areas are that they are pinned. If they are not, they
> > are actually rather useless.
> 
> The difference to unpinned areas is that pinned areas won't be kicked out to
> satisfy another allocation.

It depends on how you see it. For example in the new DRI memory manager we
implement the flags DRM_NO_MOVE and DRM_NO_EVICT. 

The old X allocator used with XAA implements  (NO_MOVE |
NOTIFY_IF_CONTENT_MAY_BE_LOST) semantics, 
and the VIA dri allocator implements (NO_MOVE | NO_EVICT) semantics.

The EXA allocator just implements (DONT_EVICT_THAT_OFTEN) which IMHO is useless.

> 
> > There is no way, for example, to tell an XvMC dri client that the surface
> offset has suddenly changed. Which makes me forced to do
> > a separate fb area for XvMC surfaces, or have it allocate only out of DRI space
> > which is a HUGE waste of resources.
> > 
> > If the content is lost, fine, the X server can always reload that, but the
> > framebuffer offset should not change. 
> 
> Dynamically changing the EXA offscreen memory range as described in comment #5
> might work for this, but the disabling and re-enabling of FB access might cause
> the display to noticeably stop updating for a bit.

Nope, it won't work. XvMC needs to be able to allocate from all of the memory
space. Due to memory size restrictions, you cannot take out an area (the EXA
managed pool) and say "XvMC cannot allocate from here", unless the EXA managed
pool is allowed to become arbitrarily small. The only thing accomplished would
be a more sluggish and constricted system, and I would have to implement yet
another allocator to manage the XvMC area.

Note that EXA itself allocates no pinned areas, so there's no need at all for
EXA to free pinned areas to make sure it preserves its memory contents.

A driver using both EXA mm, XAA mm and DRI mm would have to have memory
preservation implemented for XAA and DRI areas anyway (if it cares), so why do
it separately and in another way for EXA?

> 
> You could also enforce constant offsets by re-allocating the pinned areas in a
> driver EnableDisableFBAccess wrapper.
>

I would have to do this to reload the contents, but I cannot retain the offsets. 

> 
> > If we want to have general or user-supplied EXA allocators in the future, I
> > think EXA needs to do like XAA. Have a enableDisableFBAccess function that swaps
> > out _only_ the pixmaps managed by EXA and let the driver manage its own areas
> > from enterVT / leaveVT.
> 
> Quite frankly, I wouldn't take 'XAA does it like this' as a justification for
> anything. :) Sounds like an XAA bug to me if it just ignores the fact that all
> of video RAM may be wiped between disabling and re-enabling FB access.
>


It doesn't. It swaps out its own pixmaps, leaving all other clients to swap out
what they have allocated. XAA does what it is supposed to do IMHO. The EXA
memory manager is playing big brother :). There is a clear distinction between
the old memory manager and XAA, and I think the same distiction should be
implemented for EXA. That means EXA (not the underlying memory manager) keeps
track of what areas it has allocated (linked list) and throws them out on
disableFBAccess. The drivers keep track on what they have allocated and act
accordingly. 

In fact, the SiS driver is also broken with respect to this, It looses it's
scratch area without really caring to disable uploadFromScratch or reallocating it.

No other driver cares to reload the contents of a lost pinned area, but again,
they are only using it for video surfaces, which I guess will be disabled upon
VT switch.


> 
> Anyway, I'm adding Eric Anholt to the CC list as I'm curious if he has any
> thoughts on this.

Great. Eric, it would be great to hear your opinion.

(In reply to comment #10)
> It depends on how you see it.

I was just referring to how things are with EXA ATM.

Don't get me wrong, I agree that there are unfortunate limitations, but I'm not
sure changing the semantics like this is a good solution. Adding a new area
state such as ExaOffscreenNoSave might be better, e.g.

In the end, I think the best solution would be for everything including EXA to
use a unified memory manager.


> Note that EXA itself allocates no pinned areas, so there's no need at all for
> EXA to free pinned areas to make sure it preserves its memory contents.

What if there's code relying on the current semantics to preserve its offscreen
contents?


> No other driver cares to reload the contents of a lost pinned area, but again,
> they are only using it for video surfaces, which I guess will be disabled upon
> VT switch.

Yes, I think this is a good example of how the current semantics are useful,
however limited they are.

The current EXA behavior of swapping out on vt switch is an artifact of its
roots in kdrive, which entirely unmaps/remaps the framebuffer on vt switch.  The
behaviour I think would be better would be for the driver to signal that it
wants the allocations left alone at vt switch (since I think radeon expects the
current behaviour), and the driver at Leave/EnterVT does whatever
saving/restoring is necessary, or even deallocation/reallocation if it wants.

Of course, the unified memory manager is even more desired than fixing EXA's
lame memory manager, but until that time comes we should probably implement this.

Fix pushed.