Re: [Spice-devel] [PATCH 1/3] drm/qxl: allow both PRIV and VRAM placement for QXL_GEM_DOMAIN_SURFACE

[Gerd Hoffmann]

On Thu, Dec 06, 2018 at 09:39:54AM -0500, Frediano Ziglio wrote:
> > 
> > On Thu, Dec 06, 2018 at 05:55:58AM -0500, Frediano Ziglio wrote:
> > > 
> > > > qxl surfaces (used for framebuffers and gem objects) can live in both
> > > > VRAM and PRIV ttm domains.  Update placement setup to include both.  Put
> > > > PRIV first in the list so it is preferred, so VRAM will have more room
> > > > for objects which must be allocated there.
> > > > 
> > > > Signed-off-by: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> > > 
> > > I remember these kind of changes in the past made migration
> > > fails. I proposed similar patches years ago and they were rejected
> > > for these reasons.
> > 
> > Pointer?
> 
> I think is this:
> https://patchwork.kernel.org/patch/7374931/

Ok, problem is you are doing it for both QXL_GEM_DOMAIN_VRAM and
QXL_GEM_DOMAIN_SURFACE.  Surfaces can be in both VRAM and PRIV ttm
domains, so for the later this is fine.  Most other allocations must be
in VRAM ttm domain though, so allowing PRIV for QXL_GEM_DOMAIN_VRAM is
*not* ok.

Noticed I need patch 1/2 of that series, otherwise things will break
when we run out of space in PRIV domain and surfaces are allocated in
VRAM.

> > Well, you have to be careful what object you are allocating.  Surfaces
> > can be in both PRIV and VRAM.  Everything else (qxl commands, monitors
> > config, ...) must be in VRAM.
> > 
> > > Looks like we are improving QXL, so that means we are actively working
> > > on it.
> > 
> > Well, I'm just trying make qxl behave better with wayland.
> > 
> 
> As far as I remember the Linux kernel driver simulates the frame buffer
> swap with drawings which cause more memory copies.

Yes.  wayland renders into a dumb gem bo (backed by a qxl surface),
Actual primary surface is a shadow bo though.  Display updates are done
by sending a draw command for the primary surface, with the pixel data
coming from the dumb gem bo.

We could maybe optimize that, by having the image chunk point directly
to the dumb gem bo instead of allocating memory and memcpy'ing the pixel
data.

I don't feel like putting too much effort into qxl performance
optimization though.  The time is better spent on virtio-gpu I think.

BTW: should I send virtio-gpu kernel patches to spice-devel too?

> Not also sure if this workaround make the server consume more network
> bandwidth.

Doesn't make much of a difference I think.  In case we improve qxl/spice
to support (a) one surface per monitor and (b) pageflips so we don't
need the shadow bo, then we would still need to send the pixel data.

> Are we supporting multiple monitors for Wayland?

I have patches for that.  Which basically extend the shadow logic, so
the shadow bo used as primary surface will be big enough that all
heads/monitors/crtcs fit in.  qxl/spice has a single surface then, while
userspace (i.e. wayland) can use one dumb gem bo per head/monitor/crtc.

Guess I should re-send them with spice-devel cc'ed,

> > Main advantage is that it doesn't need qxl device changes, so it works
> > on old hosts too.  But, yes, we can consider to also modernize spice
> > protocol and qxl device.
> 
> On the other hand we faced some bugs due these workarounds so we end up
> with a solution that is less optimal than before and potentially
> with more bugs to fix.

Ahem, well, not really.

Adding support for atomic modesetting to qxl caused some regressions
indeed.  Those should be fixed meanwhile.

Anything which was done in the qxl driver for wayland (most notably the
shadow logic) improved the situation for wayland.  wayland is pretty
much unusable without shadowing.  I'm not aware of any xorg regressions
caused by dumb bo shadowing.  xorg doesn't use dumb bo's in the first
place, and it composes its own qxl drawing commands instead of expecting
the qxl kms driver handle the update on pageflip.

cheers,
  Gerd