Message ID | 20181206104638.23330-2-kraxel@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm/qxl: tweak bo allocation | expand |
> 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@redhat.com> 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. Why now they are safe? Looks like we are improving QXL, so that means we are actively working on it. Should we not then thinking about moving feature in the proper places (like spice-server for atomic mode setting instead of implementin work around) ?? > --- > drivers/gpu/drm/qxl/qxl_object.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/qxl/qxl_object.c > b/drivers/gpu/drm/qxl/qxl_object.c > index 91f3bbc73e..f7f9f4f4fe 100644 > --- a/drivers/gpu/drm/qxl/qxl_object.c > +++ b/drivers/gpu/drm/qxl/qxl_object.c > @@ -58,10 +58,10 @@ void qxl_ttm_placement_from_domain(struct qxl_bo *qbo, > u32 domain, bool pinned) > > qbo->placement.placement = qbo->placements; > qbo->placement.busy_placement = qbo->placements; > - if (domain == QXL_GEM_DOMAIN_VRAM) > - qbo->placements[c++].flags = TTM_PL_FLAG_CACHED | TTM_PL_FLAG_VRAM | > pflag; > if (domain == QXL_GEM_DOMAIN_SURFACE) > qbo->placements[c++].flags = TTM_PL_FLAG_CACHED | TTM_PL_FLAG_PRIV | > pflag; > + if (domain == QXL_GEM_DOMAIN_SURFACE || domain == QXL_GEM_DOMAIN_VRAM) > + qbo->placements[c++].flags = TTM_PL_FLAG_CACHED | TTM_PL_FLAG_VRAM | > pflag; > if (domain == QXL_GEM_DOMAIN_CPU) > qbo->placements[c++].flags = TTM_PL_MASK_CACHING | TTM_PL_FLAG_SYSTEM | > pflag; > if (!c) Frediano
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@redhat.com> > > 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? > Why now they are safe? 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. > Should we not then thinking about moving feature in the proper > places (like spice-server for atomic mode setting instead of implementin > work around) ?? 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. cheers, Gerd
> > 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@redhat.com> > > > > 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/ I think all started with Windows where we have: https://gitlab.freedesktop.org/spice/win32/qxl-wddm-dod/commit/0214d5ceda3f0da94de3813fc902150d497c6b26 https://gitlab.freedesktop.org/spice/win32/qxl-wddm-dod/commit/54a719e14f1204143da2c64f8a2aaee4fe5cd7d6 > > Why now they are safe? > > 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. Not also sure if this workaround make the server consume more network bandwidth. Are we supporting multiple monitors for Wayland? > > Should we not then thinking about moving feature in the proper > > places (like spice-server for atomic mode setting instead of implementin > > work around) ?? > > 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. > > cheers, > Gerd > > 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. At the end we sell to customer a worst product. Frediano
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@redhat.com> > > > > > > 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
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c index 91f3bbc73e..f7f9f4f4fe 100644 --- a/drivers/gpu/drm/qxl/qxl_object.c +++ b/drivers/gpu/drm/qxl/qxl_object.c @@ -58,10 +58,10 @@ void qxl_ttm_placement_from_domain(struct qxl_bo *qbo, u32 domain, bool pinned) qbo->placement.placement = qbo->placements; qbo->placement.busy_placement = qbo->placements; - if (domain == QXL_GEM_DOMAIN_VRAM) - qbo->placements[c++].flags = TTM_PL_FLAG_CACHED | TTM_PL_FLAG_VRAM | pflag; if (domain == QXL_GEM_DOMAIN_SURFACE) qbo->placements[c++].flags = TTM_PL_FLAG_CACHED | TTM_PL_FLAG_PRIV | pflag; + if (domain == QXL_GEM_DOMAIN_SURFACE || domain == QXL_GEM_DOMAIN_VRAM) + qbo->placements[c++].flags = TTM_PL_FLAG_CACHED | TTM_PL_FLAG_VRAM | pflag; if (domain == QXL_GEM_DOMAIN_CPU) qbo->placements[c++].flags = TTM_PL_MASK_CACHING | TTM_PL_FLAG_SYSTEM | pflag; if (!c)
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@redhat.com> --- drivers/gpu/drm/qxl/qxl_object.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)