Re: [PATCH] drm/qxl: fix NULL dereference in qxl_add_mode
On Fri, Mar 01, 2024 at 11:55:11AM +0300, Aleksandr Burakov wrote:
> Return value of a function 'drm_cvt_mode' is dereferenced without
> checking for NULL but drm_mode_create() in drm_cvt_mode() may
> return NULL value in case of memory allocation error.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: 1b043677d4be ("drm/qxl: add qxl_add_mode helper function")
> Signed-off-by: Aleksandr Burakov
> ---
> drivers/gpu/drm/qxl/qxl_display.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c
> b/drivers/gpu/drm/qxl/qxl_display.c
> index a152a7c6db21..447532c29e02 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -236,8 +236,10 @@ static int qxl_add_mode(struct drm_connector *connector,
> return 0;
>
> mode = drm_cvt_mode(dev, width, height, 60, false, false, false);
> - if (preferred)
> + if (preferred && mode)
> mode->type |= DRM_MODE_TYPE_PREFERRED;
> + else
> + return 0;
> mode->hdisplay = width;
That doesn't fix the NULL pointer dereference in case "preferred" is
false.
I'd suggest "if (!mode) return 0" instead.
Re: [Spice-devel] [PATCH v5 09/44] drm: handle HAS_IOPORT dependencies
Hi, > > There is also a direct and hard coded use in cirrus.c which according to > > the comment is only necessary during resume. Let's just skip this as > > for example s390 which doesn't have I/O port support also doesen't > > support suspend/resume. > > I think we should consider making cirrus depend on HAS_IOPORT. The driver is > only for qemu's cirrus emulation, which IIRC can only be enabled for i586. Agree. cirrus is x86 only (both i386 / x86_64 though). Just require HAS_IOPORT and be done with it. > And it has all been deprecated long ago. The fact that cirrus used to be the qemu default for many years is pretty much the only reason it is still somewhat relevant today ... take care, Gerd
Re: [Spice-devel] [PATCH] drm/qxl: fix the suspend/resume issue on qxl device
On Wed, Sep 07, 2022 at 05:44:23PM +0800, Zongmin Zhou wrote: > > From: Zongmin Zhou > > Details: > Currently, when trying to suspend and resume with qxl device， > there are some error messages after resuming, > eventually caused to black screen and can't be recovered. [ analysis snipped ] > Let's fix this by reset io and remove the qxl_ring_init_hdr calling. Pushed to drm-misc-next thanks, Gerd
Re: [Spice-devel] [PATCH v2 4/8] drm/qxl: Use the hotspot properties from cursor planes
On Mon, Jul 11, 2022 at 11:32:42PM -0400, Zack Rusin wrote: > From: Zack Rusin > > Atomic modesetting got support for mouse hotspots via the hotspot > properties. Port the legacy kms hotspot handling to the new properties > on cursor planes. > > Signed-off-by: Zack Rusin > Cc: Dave Airlie > Cc: Gerd Hoffmann > Cc: Daniel Vetter > Cc: virtualizat...@lists.linux-foundation.org > Cc: spice-devel@lists.freedesktop.org Reviewed-by: Gerd Hoffmann
Re: [Spice-devel] 回复: Re: 回复: Re: 回复: Re: 回复: Re: [PATCH v1 1/2] drm/qxl: replace ioremap by ioremap_cache on arm64
On Thu, Mar 24, 2022 at 06:34:02PM +0800, liuco...@kylinos.cn wrote: >ok, thanks, a lot of our customer use qxl on x86 before, so it still need >to supoort qxl on arm64. Well, qxl isn't the best choice even on x86. The main advantage it offers (2d acceleration) is basically useless today because pretty much everything moved on to use 3d acceleration instead. So qxl ends up being used as dumb framebuffer with software 3d rendering. So, I'm still recommending to just use virtio-gpu ... take care, Gerd
Re: [Spice-devel] 回复: Re: 回复: Re: 回复: Re: [PATCH v1 1/2] drm/qxl: replace ioremap by ioremap_cache on arm64
On Thu, Mar 24, 2022 at 10:20:40AM +0100, Christian König wrote: > Hi Cong, > > when I understand Robin correctly all mapping (host, guest, kernel, > userspace etc..) must have the same caching attributes unless you use the > S2FWB feature introduced with Armv8.4. > > If you don't follow those rules you usually run into coherency issues or > even worse system hangs. So you not only need to adjust the kernel mappings, > but also the function for userspace mappings to follow those rules. That matches my understanding. For qxl specifically: when using the xork qxl driver getting the userspace mappings right is essential because userspace will write qxl command buffers then. When using the xorg modesetting driver or wayland the worst thing happening would be display corruption because userspace will only map dumb bo's for pixel data. I'm wondering though why you are keen on getting qxl work instead of just using virtio-gpu? take care, Gerd
Re: [Spice-devel] [PATCH v1 1/2] drm/qxl: replace ioremap by ioremap_cache on arm64
On Wed, Mar 23, 2022 at 09:45:13AM +, Robin Murphy wrote: > On 2022-03-23 07:15, Christian König wrote: > > Am 22.03.22 um 10:34 schrieb Cong Liu: > > > qxl use ioremap to map ram_header and rom, in the arm64 implementation, > > > the device is mapped as DEVICE_nGnRE, it can not support unaligned > > > access. > > > > Well that some ARM boards doesn't allow unaligned access to MMIO space > > is a well known bug of those ARM boards. > > > > So as far as I know this is a hardware bug you are trying to workaround > > here and I'm not 100% sure that this is correct. > > No, this one's not a bug. The Device memory type used for iomem mappings is > *architecturally* defined to enforce properties like aligned accesses, no > speculation, no reordering, etc. If something wants to be treated more like > RAM than actual MMIO registers, then ioremap_wc() or ioremap_cache() is the > appropriate thing to do in general (with the former being a bit more > portable according to Documentation/driver-api/device-io.rst). Well, qxl is a virtual device, so it *is* ram. I'm wondering whenever qxl actually works on arm? As far I know all virtual display devices with (virtual) pci memory bars for vram do not work on arm due to the guest mapping vram as io memory and the host mapping vram as normal ram and the mapping attribute mismatch causes caching troubles (only noticeable on real arm hardware, not in emulation). Did something change here recently? take care, Gerd
Re: [Spice-devel] Discuss about camera redirection in SPICE
Hi, > Actually, during I developed the QEMU camera subsystem and UVC emulation, I > referred to both UVC and v4l2. > > Detailed type definition and function declaration in include/camera/camera.h > of this patch: > https://patchwork.kernel.org/project/qemu-devel/patch/20211227142734.691900-2-pizhen...@bytedance.com/ > > I suppose the API may looks like this(of cause, detailed implementation > needs take some ideas from audio). Could you please take a look at it? Looks sane from a quick glance. take care, Gerd
Re: [Spice-devel] Discuss about camera redirection in SPICE
Hi, > Camera redirection through the USB redirection protocol seems feasible. But > I have several concerns: > 1, UVC started to support H.264 and VP8 since version 1.5, old version OS > has no support(Link https://en.wikipedia.org/wiki/USB_video_device_class). > > 2, Even guest side supports UVC 1.5, the camera App still has a chance to > select which format to use. We can control this from hypervisor side. The camera could offer vp8 as only supported codec ... But, yes, using usb redirection will loose some flexibility because you can't recode the video frames then. > 3, I noticed that USB emulation uses a lot of CPU, so I also have a plan to > introduce virtio camera to reduce the CPU utilization. uhci and ehci are pretty bad indeed. xhci should behave noticeable better. > So from the point of my view, I prefer a common camera redirection > protocol(event a hard work to do, but I can support it for a long time). I think you can take some ideas from the audio protocol (start/stop stream, ...). Define controls (brighness etc), probably best to follow uvc or v4l2 here. Negotiate video format capabilities, so there is the option that server and client agree on some future codec when supported on both ends. take care, Gerd
Re: [Spice-devel] Discuss about camera redirection in SPICE
Hi, > Although USB redirection has already provided a solution to use a remote > webcam, I notice that it uses a heavy network(1280*720@30FPS in MJPEG uses > 5MB/s+). I have tested several webcam, and all of them don't support h264. > So I'd like to develop camera redirection in SPICE with h264 support, and > expect to reduce the network bandwidth(300K/s may be enough). Well, one option would be to add usb webcam emulation to the spice client, simliar to cdrom redirection (which emulates an usb cdrom drive under the hood). Advantage: works without spice protocol changes as you can simply tunnel everything through the usb redirection protocol. I'd also recommend to look for another video codec (if possible, not sure what the usb webcam spec allows). H.264 is a patent minefield, which makes it rather difficult to use in open source projects. You'll end up with a lot of legal problems when it comes to software distribution. vp8/9 would be a much better choice. take care, Gerd
Re: [Spice-devel] [RFC 26/32] drm: handle HAS_IOPORT dependencies
On Mon, Dec 27, 2021 at 05:43:11PM +0100, Niklas Schnelle wrote: > In a future patch HAS_IOPORT=n will result in inb()/outb() and friends > not being declared. We thus need to add HAS_IOPORT as dependency for > those drivers using them. There is also a direct and hard coded use in > cirrus.c which according to the comment is only necessary during resume. > Let's just skip this as for example s390 which doesn't have I/O port > support also doesen't support suspend/resume. > config DRM_BOCHS > tristate "DRM Support for bochs dispi vga interface (qemu stdvga)" > depends on DRM && PCI && MMU > + depends on HAS_IOPORT > select DRM_KMS_HELPER > select DRM_VRAM_HELPER > select DRM_TTM On devices with an mmio bar the driver works just fine without inb/outb, see bochs->mmio checks in bochs.c take care, Gerd
Re: [Spice-devel] [PATCH] drm/qxl: Remove empty qxl_gem_prime_mmap()
On Thu, Jun 24, 2021 at 11:05:00AM +0200, Thomas Zimmermann wrote: > The function qxl_gem_prime_mmap() returns an error. The two callers > of gem_prime_mmap are drm_fbdev_fb_mmap() and drm_gem_dmabuf_mmap(), > which both already handle NULL-callbacks with an error code. So clear > gem_prime_mmap in qxl and remove qxl_gem_prime_mmap(). Reviewed-by: Gerd Hoffmann ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] drm: qxl: ensure surf.data is ininitialized
On Tue, Jun 08, 2021 at 05:13:13PM +0100, Colin King wrote:
> From: Colin Ian King
>
> The object surf is not fully initialized and the uninitialized
> field surf.data is being copied by the call to qxl_bo_create
> via the call to qxl_gem_object_create. Set surf.data to zero
> to ensure garbage data from the stack is not being copied.
>
> Addresses-Coverity: ("Uninitialized scalar variable")
> Fixes: f64122c1f6ad ("drm: add new QXL driver. (v1.4)")
> Signed-off-by: Colin Ian King
Pushed to drm-misc-next.
thanks,
Gerd
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH 1/2] drm/qxl: drop redundant code
Not needed, qxl_io_destroy_primary() does that for us.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_display.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index a7637e79cb42..be5183733c1b 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -677,10 +677,8 @@ static void qxl_primary_atomic_disable(struct drm_plane
*plane,
if (bo->shadow)
bo = bo->shadow;
- if (bo->is_primary) {
+ if (bo->is_primary)
qxl_io_destroy_primary(qdev);
- bo->is_primary = false;
- }
}
}
--
2.31.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH 2/2] drm/qxl: balance dumb_shadow_bo pin
The shadow bo is created in pinned state, so we have to unpin it when
dropping the reference. Otherwise ttm is unhappy and throws a WARN()
on release.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_display.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index be5183733c1b..9e0a1e836011 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -801,6 +801,7 @@ static void qxl_prepare_shadow(struct qxl_device *qdev,
struct qxl_bo *user_bo,
qdev->dumb_shadow_bo->surf.width != surf.width ||
qdev->dumb_shadow_bo->surf.height != surf.height) {
if (qdev->dumb_shadow_bo) {
+ qxl_bo_unpin(qdev->dumb_shadow_bo);
drm_gem_object_put
(>dumb_shadow_bo->tbo.base);
qdev->dumb_shadow_bo = NULL;
--
2.31.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] qxl: Fix uninitialised struct field head.surface_id
On Thu, Mar 04, 2021 at 09:49:28AM +, Colin King wrote:
> From: Colin Ian King
>
> The surface_id struct field in head is not being initialized and
> static analysis warns that this is being passed through to
> dev->monitors_config->heads[i] on an assignment. Clear up this
> warning by initializing it to zero.
>
> Addresses-Coverity: ("Uninitialized scalar variable")
> Fixes: a6d3c4d79822 ("qxl: hook monitors_config updates into crtc, not
> encoder.")
> Signed-off-by: Colin Ian King
Pushed to drm-misc-fixes.
thanks,
Gerd
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v2 10/11] drm/qxl: rework cursor plane
Hi, > > Well. I suspect I could easily spend a month cleaning up and party > > redesign the qxl driver (specifically qxl_draw.c + qxl_image.c). > > > > I'm not sure I'll find the time to actually do that anytime soon. > > I have plenty of other stuff on my TODO list, and given that the > > world is transitioning to virtio-gpu the priority for qxl isn't > > that high. > > Well, in that case: > > Acked-by: Thomas Zimmermann > > for patches 9 and 10. Having the vmap calls fixed is at least worth it. Thanks. Any chance you can ack 8/11 too (only patch missing an ack). take care, Gerd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v2 10/11] drm/qxl: rework cursor plane
Hi, > I'm still trying to wrap my head around the qxl cursor code. > > Getting vmap out of the commit tail is good, but I feel like this isn't > going in the right direction overall. > > In ast, these helper functions were only good when converting the drvier to > atomic modesetting. So I removed them in the latst patchset and did all the > updates in the plane helpers directly. I see the helper functions more as a way to get some structure into the code flow. The callbacks are easier to read if they just call helper functions for stuff which needs more than a handful lines of code (patch 9/11 exists for the same reason). The helpers also make it easier move work from one callback to another, but that is just a useful side-effect. I had considered making that two separate patches, one factor out code into functions and one moving the calls. Turned out to not be that easy though, because the old qxl_cursor_atomic_update() code was a rather hairy mix of qxl_create_cursor() + qxl_primary_apply_cursor() + qxl_primary_move_cursor(). > For cursor_bo itself, it seems to be transitional state that is only used > during the plane update and crtc update . It should probably be stored in a > plane-state structure. > > Some of the primary plane's functions seem to deal with cursor handling. > What's the role of the primary plane in cursor handling? It's a quirk. The qxl device will forget the cursor state on qxl_io_create_primary(), so I have to remember the cursor state and re-establish it by calling qxl_primary_apply_cursor() again. So I'm not sure sticking this into plane state would work. Because of the quirk this is more than just a handover from prepare to commit. > For now, I suggest to merge patch 1 to 8 and 11; and move the cursor patches > into a new patchset. I can merge 1-8, but 11 has to wait until the cursor is sorted. There is a reason why 11 is last in the series ;) > I'd like ot hear Daniel's opinion on this. Do you have > further plans here? Well. I suspect I could easily spend a month cleaning up and party redesign the qxl driver (specifically qxl_draw.c + qxl_image.c). I'm not sure I'll find the time to actually do that anytime soon. I have plenty of other stuff on my TODO list, and given that the world is transitioning to virtio-gpu the priority for qxl isn't that high. So, no, I have no short-term plans for qxl beyond fixing pins + reservations + lockdep. take care, Gerd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v2 01/11] drm/qxl: properly handle device init failures
Specifically do not try release resources which where
not allocated in the first place.
Cc: Tong Zhang
Tested-by: Tong Zhang
Signed-off-by: Gerd Hoffmann
Acked-by: Thomas Zimmermann
---
drivers/gpu/drm/qxl/qxl_display.c | 3 +++
drivers/gpu/drm/qxl/qxl_kms.c | 4
2 files changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index c326412136c5..ec50d2cfd4e1 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -1183,6 +1183,9 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev)
{
int ret;
+ if (!qdev->monitors_config_bo)
+ return 0;
+
qdev->monitors_config = NULL;
qdev->ram_header->monitors_config = 0;
diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index 66d74aaaee06..4dc5ad13f12c 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -288,6 +288,10 @@ void qxl_device_fini(struct qxl_device *qdev)
{
int cur_idx;
+ /* check if qxl_device_init() was successful (gc_work is initialized
last) */
+ if (!qdev->gc_work.func)
+ return;
+
for (cur_idx = 0; cur_idx < 3; cur_idx++) {
if (!qdev->current_release_bo[cur_idx])
continue;
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v2 11/11] drm/qxl: add lock asserts to qxl_bo_vmap_locked + qxl_bo_vunmap_locked
Try avoid re-introducing locking bugs.
Signed-off-by: Gerd Hoffmann
Acked-by: Thomas Zimmermann
---
drivers/gpu/drm/qxl/qxl_object.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index 82c3bf195ad6..6e26d70f2f07 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -162,6 +162,8 @@ int qxl_bo_vmap_locked(struct qxl_bo *bo, struct
dma_buf_map *map)
{
int r;
+ dma_resv_assert_held(bo->tbo.base.resv);
+
if (bo->kptr) {
bo->map_count++;
goto out;
@@ -236,6 +238,8 @@ void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev,
void qxl_bo_vunmap_locked(struct qxl_bo *bo)
{
+ dma_resv_assert_held(bo->tbo.base.resv);
+
if (bo->kptr == NULL)
return;
bo->map_count--;
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v2 05/11] drm/qxl: rename qxl_bo_kmap -> qxl_bo_vmap_locked
Append _locked to Make clear that these functions should be called with reserved bo's only. While being at it also rename kmap -> vmap. No functional change. Signed-off-by: Gerd Hoffmann --- drivers/gpu/drm/qxl/qxl_object.h | 4 ++-- drivers/gpu/drm/qxl/qxl_display.c | 14 +++--- drivers/gpu/drm/qxl/qxl_draw.c| 8 drivers/gpu/drm/qxl/qxl_image.c | 2 +- drivers/gpu/drm/qxl/qxl_object.c | 8 drivers/gpu/drm/qxl/qxl_prime.c | 4 ++-- 6 files changed, 20 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h index dc1659e717f1..2495e5cdf353 100644 --- a/drivers/gpu/drm/qxl/qxl_object.h +++ b/drivers/gpu/drm/qxl/qxl_object.h @@ -64,8 +64,8 @@ extern int qxl_bo_create(struct qxl_device *qdev, u32 priority, struct qxl_surface *surf, struct qxl_bo **bo_ptr); -extern int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map); -extern void qxl_bo_kunmap(struct qxl_bo *bo); +int qxl_bo_vmap_locked(struct qxl_bo *bo, struct dma_buf_map *map); +void qxl_bo_vunmap_locked(struct qxl_bo *bo); void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, int page_offset); void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, void *map); extern struct qxl_bo *qxl_bo_ref(struct qxl_bo *bo); diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index a1b5cc5918bc..bfcc93089a94 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -600,7 +600,7 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, user_bo = gem_to_qxl_bo(obj); /* pinning is done in the prepare/cleanup framevbuffer */ - ret = qxl_bo_kmap(user_bo, _map); + ret = qxl_bo_vmap_locked(user_bo, _map); if (ret) goto out_free_release; user_ptr = user_map.vaddr; /* TODO: Use mapping abstraction properly */ @@ -619,7 +619,7 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, if (ret) goto out_unpin; - ret = qxl_bo_kmap(cursor_bo, _map); + ret = qxl_bo_vmap_locked(cursor_bo, _map); if (ret) goto out_backoff; if (cursor_map.is_iomem) /* TODO: Use mapping abstraction properly */ @@ -638,8 +638,8 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, cursor->chunk.prev_chunk = 0; cursor->chunk.data_size = size; memcpy(cursor->chunk.data, user_ptr, size); - qxl_bo_kunmap(cursor_bo); - qxl_bo_kunmap(user_bo); + qxl_bo_vunmap_locked(cursor_bo); + qxl_bo_vunmap_locked(user_bo); cmd = (struct qxl_cursor_cmd *) qxl_release_map(qdev, release); cmd->u.set.visible = 1; @@ -681,7 +681,7 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, out_free_bo: qxl_bo_unref(_bo); out_kunmap: - qxl_bo_kunmap(user_bo); + qxl_bo_vunmap_locked(user_bo); out_free_release: qxl_release_free(qdev, release); return; @@ -1163,7 +1163,7 @@ int qxl_create_monitors_object(struct qxl_device *qdev) if (ret) return ret; - qxl_bo_kmap(qdev->monitors_config_bo, ); + qxl_bo_vmap_locked(qdev->monitors_config_bo, ); qdev->monitors_config = qdev->monitors_config_bo->kptr; qdev->ram_header->monitors_config = @@ -1189,7 +1189,7 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev) qdev->monitors_config = NULL; qdev->ram_header->monitors_config = 0; - qxl_bo_kunmap(qdev->monitors_config_bo); + qxl_bo_vunmap_locked(qdev->monitors_config_bo); ret = qxl_bo_unpin(qdev->monitors_config_bo); if (ret) return ret; diff --git a/drivers/gpu/drm/qxl/qxl_draw.c b/drivers/gpu/drm/qxl/qxl_draw.c index 7b7acb910780..7d27891e87fa 100644 --- a/drivers/gpu/drm/qxl/qxl_draw.c +++ b/drivers/gpu/drm/qxl/qxl_draw.c @@ -48,7 +48,7 @@ static struct qxl_rect *drawable_set_clipping(struct qxl_device *qdev, struct qxl_clip_rects *dev_clips; int ret; - ret = qxl_bo_kmap(clips_bo, ); + ret = qxl_bo_vmap_locked(clips_bo, ); if (ret) return NULL; dev_clips = map.vaddr; /* TODO: Use mapping abstraction properly */ @@ -202,7 +202,7 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev, if (ret) goto out_release_backoff; - ret = qxl_bo_kmap(bo, _map); + ret = qxl_bo_vmap_locked(bo, _map); if (ret) goto out_release_backoff; surface_base = surface_map.vaddr; /* TODO: Use mapping abstraction properl
[Spice-devel] [PATCH v2 07/11] drm/qxl: fix prime vmap
Use the correct vmap variant. We don't have a reservation here,
so we can't use the _locked version.
Signed-off-by: Gerd Hoffmann
Acked-by: Thomas Zimmermann
---
drivers/gpu/drm/qxl/qxl_prime.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_prime.c b/drivers/gpu/drm/qxl/qxl_prime.c
index 2bebe662516f..0628d1cc91fe 100644
--- a/drivers/gpu/drm/qxl/qxl_prime.c
+++ b/drivers/gpu/drm/qxl/qxl_prime.c
@@ -59,7 +59,7 @@ int qxl_gem_prime_vmap(struct drm_gem_object *obj, struct
dma_buf_map *map)
struct qxl_bo *bo = gem_to_qxl_bo(obj);
int ret;
- ret = qxl_bo_vmap_locked(bo, map);
+ ret = qxl_bo_vmap(bo, map);
if (ret < 0)
return ret;
@@ -71,7 +71,7 @@ void qxl_gem_prime_vunmap(struct drm_gem_object *obj,
{
struct qxl_bo *bo = gem_to_qxl_bo(obj);
- qxl_bo_vunmap_locked(bo);
+ qxl_bo_vunmap(bo);
}
int qxl_gem_prime_mmap(struct drm_gem_object *obj,
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v2 08/11] drm/qxl: fix monitors object vmap
Use the correct vmap variant. We don't hold a reservation here, so we can't use the _locked variant. We can drop the pin because qxl_bo_vmap will do that for us. Signed-off-by: Gerd Hoffmann --- drivers/gpu/drm/qxl/qxl_display.c | 7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index bfcc93089a94..f106da917863 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -1159,12 +1159,10 @@ int qxl_create_monitors_object(struct qxl_device *qdev) } qdev->monitors_config_bo = gem_to_qxl_bo(gobj); - ret = qxl_bo_pin(qdev->monitors_config_bo); + ret = qxl_bo_vmap(qdev->monitors_config_bo, ); if (ret) return ret; - qxl_bo_vmap_locked(qdev->monitors_config_bo, ); - qdev->monitors_config = qdev->monitors_config_bo->kptr; qdev->ram_header->monitors_config = qxl_bo_physical_address(qdev, qdev->monitors_config_bo, 0); @@ -1189,8 +1187,7 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev) qdev->monitors_config = NULL; qdev->ram_header->monitors_config = 0; - qxl_bo_vunmap_locked(qdev->monitors_config_bo); - ret = qxl_bo_unpin(qdev->monitors_config_bo); + ret = qxl_bo_vunmap(qdev->monitors_config_bo); if (ret) return ret; -- 2.29.2 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v2 09/11] drm/qxl: move shadow handling to new qxl_prepare_shadow()
Pure code motion, no functional change.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_display.c | 61 +--
1 file changed, 34 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index f106da917863..b315d7484e21 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -771,13 +771,45 @@ static void qxl_calc_dumb_shadow(struct qxl_device *qdev,
DRM_DEBUG("%dx%d\n", surf->width, surf->height);
}
+static void qxl_prepare_shadow(struct qxl_device *qdev, struct qxl_bo *user_bo,
+ int crtc_index)
+{
+ struct qxl_surface surf;
+
+ qxl_update_dumb_head(qdev, crtc_index,
+user_bo);
+ qxl_calc_dumb_shadow(qdev, );
+ if (!qdev->dumb_shadow_bo ||
+ qdev->dumb_shadow_bo->surf.width != surf.width ||
+ qdev->dumb_shadow_bo->surf.height != surf.height) {
+ if (qdev->dumb_shadow_bo) {
+ drm_gem_object_put
+ (>dumb_shadow_bo->tbo.base);
+ qdev->dumb_shadow_bo = NULL;
+ }
+ qxl_bo_create(qdev, surf.height * surf.stride,
+ true, true, QXL_GEM_DOMAIN_SURFACE, 0,
+ , >dumb_shadow_bo);
+ }
+ if (user_bo->shadow != qdev->dumb_shadow_bo) {
+ if (user_bo->shadow) {
+ qxl_bo_unpin(user_bo->shadow);
+ drm_gem_object_put
+ (_bo->shadow->tbo.base);
+ user_bo->shadow = NULL;
+ }
+ drm_gem_object_get(>dumb_shadow_bo->tbo.base);
+ user_bo->shadow = qdev->dumb_shadow_bo;
+ qxl_bo_pin(user_bo->shadow);
+ }
+}
+
static int qxl_plane_prepare_fb(struct drm_plane *plane,
struct drm_plane_state *new_state)
{
struct qxl_device *qdev = to_qxl(plane->dev);
struct drm_gem_object *obj;
struct qxl_bo *user_bo;
- struct qxl_surface surf;
if (!new_state->fb)
return 0;
@@ -787,32 +819,7 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane,
if (plane->type == DRM_PLANE_TYPE_PRIMARY &&
user_bo->is_dumb) {
- qxl_update_dumb_head(qdev, new_state->crtc->index,
-user_bo);
- qxl_calc_dumb_shadow(qdev, );
- if (!qdev->dumb_shadow_bo ||
- qdev->dumb_shadow_bo->surf.width != surf.width ||
- qdev->dumb_shadow_bo->surf.height != surf.height) {
- if (qdev->dumb_shadow_bo) {
- drm_gem_object_put
- (>dumb_shadow_bo->tbo.base);
- qdev->dumb_shadow_bo = NULL;
- }
- qxl_bo_create(qdev, surf.height * surf.stride,
- true, true, QXL_GEM_DOMAIN_SURFACE, 0,
- , >dumb_shadow_bo);
- }
- if (user_bo->shadow != qdev->dumb_shadow_bo) {
- if (user_bo->shadow) {
- qxl_bo_unpin(user_bo->shadow);
- drm_gem_object_put
- (_bo->shadow->tbo.base);
- user_bo->shadow = NULL;
- }
- drm_gem_object_get(>dumb_shadow_bo->tbo.base);
- user_bo->shadow = qdev->dumb_shadow_bo;
- qxl_bo_pin(user_bo->shadow);
- }
+ qxl_prepare_shadow(qdev, user_bo, new_state->crtc->index);
}
return qxl_bo_pin(user_bo);
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v2 10/11] drm/qxl: rework cursor plane
Add helper functions to create and move the cursor.
Create the cursor_bo in prepare_fb callback, in the
atomic_commit callback we only send the update command
to the host.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_display.c | 248 --
1 file changed, 133 insertions(+), 115 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index b315d7484e21..4a3d272e8d6c 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -476,12 +476,11 @@ static int qxl_primary_atomic_check(struct drm_plane
*plane,
return qxl_check_framebuffer(qdev, bo);
}
-static int qxl_primary_apply_cursor(struct drm_plane *plane)
+static int qxl_primary_apply_cursor(struct qxl_device *qdev,
+ struct drm_plane_state *plane_state)
{
- struct drm_device *dev = plane->dev;
- struct qxl_device *qdev = to_qxl(dev);
- struct drm_framebuffer *fb = plane->state->fb;
- struct qxl_crtc *qcrtc = to_qxl_crtc(plane->state->crtc);
+ struct drm_framebuffer *fb = plane_state->fb;
+ struct qxl_crtc *qcrtc = to_qxl_crtc(plane_state->crtc);
struct qxl_cursor_cmd *cmd;
struct qxl_release *release;
int ret = 0;
@@ -505,8 +504,8 @@ static int qxl_primary_apply_cursor(struct drm_plane *plane)
cmd = (struct qxl_cursor_cmd *)qxl_release_map(qdev, release);
cmd->type = QXL_CURSOR_SET;
- cmd->u.set.position.x = plane->state->crtc_x + fb->hot_x;
- cmd->u.set.position.y = plane->state->crtc_y + fb->hot_y;
+ cmd->u.set.position.x = plane_state->crtc_x + fb->hot_x;
+ cmd->u.set.position.y = plane_state->crtc_y + fb->hot_y;
cmd->u.set.shape = qxl_bo_physical_address(qdev, qcrtc->cursor_bo, 0);
@@ -523,6 +522,113 @@ static int qxl_primary_apply_cursor(struct drm_plane
*plane)
return ret;
}
+static int qxl_primary_move_cursor(struct qxl_device *qdev,
+ struct drm_plane_state *plane_state)
+{
+ struct drm_framebuffer *fb = plane_state->fb;
+ struct qxl_crtc *qcrtc = to_qxl_crtc(plane_state->crtc);
+ struct qxl_cursor_cmd *cmd;
+ struct qxl_release *release;
+ int ret = 0;
+
+ if (!qcrtc->cursor_bo)
+ return 0;
+
+ ret = qxl_alloc_release_reserved(qdev, sizeof(*cmd),
+QXL_RELEASE_CURSOR_CMD,
+, NULL);
+ if (ret)
+ return ret;
+
+ ret = qxl_release_reserve_list(release, true);
+ if (ret) {
+ qxl_release_free(qdev, release);
+ return ret;
+ }
+
+ cmd = (struct qxl_cursor_cmd *)qxl_release_map(qdev, release);
+ cmd->type = QXL_CURSOR_MOVE;
+ cmd->u.position.x = plane_state->crtc_x + fb->hot_x;
+ cmd->u.position.y = plane_state->crtc_y + fb->hot_y;
+ qxl_release_unmap(qdev, release, >release_info);
+
+ qxl_release_fence_buffer_objects(release);
+ qxl_push_cursor_ring_release(qdev, release, QXL_CMD_CURSOR, false);
+ return ret;
+}
+
+static struct qxl_bo *qxl_create_cursor(struct qxl_device *qdev,
+ struct qxl_bo *user_bo,
+ int hot_x, int hot_y)
+{
+ static const u32 size = 64 * 64 * 4;
+ struct qxl_bo *cursor_bo;
+ struct dma_buf_map cursor_map;
+ struct dma_buf_map user_map;
+ struct qxl_cursor cursor;
+ int ret;
+
+ if (!user_bo)
+ return NULL;
+
+ ret = qxl_bo_create(qdev, sizeof(struct qxl_cursor) + size,
+ false, true, QXL_GEM_DOMAIN_VRAM, 1,
+ NULL, _bo);
+ if (ret)
+ goto err;
+
+ ret = qxl_bo_vmap(cursor_bo, _map);
+ if (ret)
+ goto err_unref;
+
+ ret = qxl_bo_vmap(user_bo, _map);
+ if (ret)
+ goto err_unmap;
+
+ cursor.header.unique = 0;
+ cursor.header.type = SPICE_CURSOR_TYPE_ALPHA;
+ cursor.header.width = 64;
+ cursor.header.height = 64;
+ cursor.header.hot_spot_x = hot_x;
+ cursor.header.hot_spot_y = hot_y;
+ cursor.data_size = size;
+ cursor.chunk.next_chunk = 0;
+ cursor.chunk.prev_chunk = 0;
+ cursor.chunk.data_size = size;
+ if (cursor_map.is_iomem) {
+ memcpy_toio(cursor_map.vaddr_iomem,
+ , sizeof(cursor));
+ memcpy_toio(cursor_map.vaddr_iomem + sizeof(cursor),
+ user_map.vaddr, size);
+ } else {
+ memcpy(cursor_map.vaddr,
+ , sizeof(cursor));
+ memcpy(cursor_map.vaddr + sizeof(cursor),
+ user_map.vaddr, size
[Spice-devel] [PATCH v2 06/11] drm/qxl: add qxl_bo_vmap/qxl_bo_vunmap
Add vmap/vunmap variants which reserve (and pin) the bo.
They can be used in case the caller doesn't hold a reservation
for the bo.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_object.h | 2 ++
drivers/gpu/drm/qxl/qxl_object.c | 36
2 files changed, 38 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h
index 2495e5cdf353..ee9c29de4d3d 100644
--- a/drivers/gpu/drm/qxl/qxl_object.h
+++ b/drivers/gpu/drm/qxl/qxl_object.h
@@ -64,7 +64,9 @@ extern int qxl_bo_create(struct qxl_device *qdev,
u32 priority,
struct qxl_surface *surf,
struct qxl_bo **bo_ptr);
+int qxl_bo_vmap(struct qxl_bo *bo, struct dma_buf_map *map);
int qxl_bo_vmap_locked(struct qxl_bo *bo, struct dma_buf_map *map);
+int qxl_bo_vunmap(struct qxl_bo *bo);
void qxl_bo_vunmap_locked(struct qxl_bo *bo);
void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, int
page_offset);
void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo,
void *map);
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index f4a015381a7f..82c3bf195ad6 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -29,6 +29,9 @@
#include "qxl_drv.h"
#include "qxl_object.h"
+static int __qxl_bo_pin(struct qxl_bo *bo);
+static void __qxl_bo_unpin(struct qxl_bo *bo);
+
static void qxl_ttm_bo_destroy(struct ttm_buffer_object *tbo)
{
struct qxl_bo *bo;
@@ -179,6 +182,25 @@ int qxl_bo_vmap_locked(struct qxl_bo *bo, struct
dma_buf_map *map)
return 0;
}
+int qxl_bo_vmap(struct qxl_bo *bo, struct dma_buf_map *map)
+{
+ int r;
+
+ r = qxl_bo_reserve(bo);
+ if (r)
+ return r;
+
+ r = __qxl_bo_pin(bo);
+ if (r) {
+ qxl_bo_unreserve(bo);
+ return r;
+ }
+
+ r = qxl_bo_vmap_locked(bo, map);
+ qxl_bo_unreserve(bo);
+ return r;
+}
+
void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev,
struct qxl_bo *bo, int page_offset)
{
@@ -223,6 +245,20 @@ void qxl_bo_vunmap_locked(struct qxl_bo *bo)
ttm_bo_vunmap(>tbo, >map);
}
+int qxl_bo_vunmap(struct qxl_bo *bo)
+{
+ int r;
+
+ r = qxl_bo_reserve(bo);
+ if (r)
+ return r;
+
+ qxl_bo_vunmap_locked(bo);
+ __qxl_bo_unpin(bo);
+ qxl_bo_unreserve(bo);
+ return 0;
+}
+
void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev,
struct qxl_bo *bo, void *pmap)
{
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v2 02/11] drm/qxl: more fence wait rework
Move qxl_io_notify_oom() call into wait condition.
That way the driver will call it again if one call
wasn't enough.
Also allows to remove the extra dma_fence_is_signaled()
check and the goto.
Fixes: 5a838e5d5825 ("drm/qxl: simplify qxl_fence_wait")
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_release.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_release.c
b/drivers/gpu/drm/qxl/qxl_release.c
index 6ed673d75f9f..579c6de10c8e 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -62,16 +62,12 @@ static long qxl_fence_wait(struct dma_fence *fence, bool
intr,
qdev = container_of(fence->lock, struct qxl_device, release_lock);
- if (dma_fence_is_signaled(fence))
- goto signaled;
-
- qxl_io_notify_oom(qdev);
if (!wait_event_timeout(qdev->release_event,
- dma_fence_is_signaled(fence),
+ (dma_fence_is_signaled(fence) ||
+(qxl_io_notify_oom(qdev), 0)),
timeout))
return 0;
-signaled:
cur = jiffies;
if (time_after(cur, end))
return 0;
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v2 03/11] drm/qxl: use ttm bo priorities
Allow to set priorities for buffer objects. Use priority 1 for surface
and cursor command releases. Use priority 0 for drawing command
releases. That way the short-living drawing commands are first in line
when it comes to eviction, making it *much* less likely that
ttm_bo_mem_force_space() picks something which can't be evicted and
throws an error after waiting a while without success.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_object.h | 1 +
drivers/gpu/drm/qxl/qxl_cmd.c | 2 +-
drivers/gpu/drm/qxl/qxl_display.c | 4 ++--
drivers/gpu/drm/qxl/qxl_gem.c | 2 +-
drivers/gpu/drm/qxl/qxl_object.c | 5 +++--
drivers/gpu/drm/qxl/qxl_release.c | 18 --
6 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h
index e60a8f88e226..dc1659e717f1 100644
--- a/drivers/gpu/drm/qxl/qxl_object.h
+++ b/drivers/gpu/drm/qxl/qxl_object.h
@@ -61,6 +61,7 @@ static inline u64 qxl_bo_mmap_offset(struct qxl_bo *bo)
extern int qxl_bo_create(struct qxl_device *qdev,
unsigned long size,
bool kernel, bool pinned, u32 domain,
+u32 priority,
struct qxl_surface *surf,
struct qxl_bo **bo_ptr);
extern int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map);
diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index 7e22a81bfb36..7b00c955cd82 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -269,7 +269,7 @@ int qxl_alloc_bo_reserved(struct qxl_device *qdev,
int ret;
ret = qxl_bo_create(qdev, size, false /* not kernel - device */,
- false, QXL_GEM_DOMAIN_VRAM, NULL, );
+ false, QXL_GEM_DOMAIN_VRAM, 0, NULL, );
if (ret) {
DRM_ERROR("failed to allocate VRAM BO\n");
return ret;
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index ec50d2cfd4e1..a1b5cc5918bc 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -799,8 +799,8 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane,
qdev->dumb_shadow_bo = NULL;
}
qxl_bo_create(qdev, surf.height * surf.stride,
- true, true, QXL_GEM_DOMAIN_SURFACE, ,
- >dumb_shadow_bo);
+ true, true, QXL_GEM_DOMAIN_SURFACE, 0,
+ , >dumb_shadow_bo);
}
if (user_bo->shadow != qdev->dumb_shadow_bo) {
if (user_bo->shadow) {
diff --git a/drivers/gpu/drm/qxl/qxl_gem.c b/drivers/gpu/drm/qxl/qxl_gem.c
index 48e096285b4c..a08da0bd9098 100644
--- a/drivers/gpu/drm/qxl/qxl_gem.c
+++ b/drivers/gpu/drm/qxl/qxl_gem.c
@@ -55,7 +55,7 @@ int qxl_gem_object_create(struct qxl_device *qdev, int size,
/* At least align on page size */
if (alignment < PAGE_SIZE)
alignment = PAGE_SIZE;
- r = qxl_bo_create(qdev, size, kernel, false, initial_domain, surf,
);
+ r = qxl_bo_create(qdev, size, kernel, false, initial_domain, 0, surf,
);
if (r) {
if (r != -ERESTARTSYS)
DRM_ERROR(
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index 705b51535492..7eada4ad217b 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -103,8 +103,8 @@ static const struct drm_gem_object_funcs qxl_object_funcs =
{
.print_info = drm_gem_ttm_print_info,
};
-int qxl_bo_create(struct qxl_device *qdev,
- unsigned long size, bool kernel, bool pinned, u32 domain,
+int qxl_bo_create(struct qxl_device *qdev, unsigned long size,
+ bool kernel, bool pinned, u32 domain, u32 priority,
struct qxl_surface *surf,
struct qxl_bo **bo_ptr)
{
@@ -137,6 +137,7 @@ int qxl_bo_create(struct qxl_device *qdev,
qxl_ttm_placement_from_domain(bo, domain);
+ bo->tbo.priority = priority;
r = ttm_bo_init_reserved(>mman.bdev, >tbo, size, type,
>placement, 0, , NULL, NULL,
_ttm_bo_destroy);
diff --git a/drivers/gpu/drm/qxl/qxl_release.c
b/drivers/gpu/drm/qxl/qxl_release.c
index 579c6de10c8e..716d706ca7f0 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -160,11 +160,12 @@ qxl_release_free(struct qxl_device *qdev,
}
static int qxl_release_bo_alloc(struct qxl_device *qdev,
- struct qxl_bo **bo)
+ struct qxl_bo **bo,
+ u32 priority)
{
[Spice-devel] [PATCH v2 04/11] drm/qxl: fix lockdep issue in qxl_alloc_release_reserved
Call qxl_bo_unpin (which does a reservation) without holding the
release_mutex lock. Fixes lockdep (correctly) warning on a possible
deadlock.
Fixes: 65ffea3c6e73 ("drm/qxl: unpin release objects")
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_release.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_release.c
b/drivers/gpu/drm/qxl/qxl_release.c
index 716d706ca7f0..f5845c96d414 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -283,7 +283,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev,
unsigned long size,
int type, struct qxl_release **release,
struct qxl_bo **rbo)
{
- struct qxl_bo *bo;
+ struct qxl_bo *bo, *free_bo = NULL;
int idr_ret;
int ret = 0;
union qxl_release_info *info;
@@ -315,8 +315,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev,
unsigned long size,
mutex_lock(>release_mutex);
if (qdev->current_release_bo_offset[cur_idx] + 1 >=
releases_per_bo[cur_idx]) {
- qxl_bo_unpin(qdev->current_release_bo[cur_idx]);
- qxl_bo_unref(>current_release_bo[cur_idx]);
+ free_bo = qdev->current_release_bo[cur_idx];
qdev->current_release_bo_offset[cur_idx] = 0;
qdev->current_release_bo[cur_idx] = NULL;
}
@@ -324,6 +323,10 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev,
unsigned long size,
ret = qxl_release_bo_alloc(qdev,
>current_release_bo[cur_idx], priority);
if (ret) {
mutex_unlock(>release_mutex);
+ if (free_bo) {
+ qxl_bo_unpin(free_bo);
+ qxl_bo_unref(_bo);
+ }
qxl_release_free(qdev, *release);
return ret;
}
@@ -339,6 +342,10 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev,
unsigned long size,
*rbo = bo;
mutex_unlock(>release_mutex);
+ if (free_bo) {
+ qxl_bo_unpin(free_bo);
+ qxl_bo_unref(_bo);
+ }
ret = qxl_release_list_add(*release, bo);
qxl_bo_unref();
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 09/10] drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks.
On Tue, Feb 16, 2021 at 02:46:21PM +0100, Thomas Zimmermann wrote: > > > Am 16.02.21 um 14:27 schrieb Thomas Zimmermann: > > Hi > > > > this is a shadow-buffered plane. Did you consider using the new helpers > > for shadow-buffered planes? They will map the user BO for you and > > provide the mapping in the plane state. > > > > From there, you should implement your own plane state on top of struct > > drm_shadow_plane_state, and also move all the other allocations and > > vmaps into prepare_fb and cleanup_fb. Most of this is not actually > > allowed in commit tails. All we'd have to do is to export the reset, > > duplicate and destroy code; similar to what > > __drm_atomic_helper_plane_reset() does. > > AFAICT the cursor_bo is used to implement double buffering for the cursor > image. > > Ideally, you can do what ast does: pre-allocate/vmap 2 BOs at the end of the > vram. Then pageflip between them in atomic_update(). Resolves all the > allocation and mapping headaches. Just waded through the ast patches. It is not that simple for qxl. You have to send a command to the virtualization host and take care of the host accessing that memory when processing the command, so you can't reuse the memory until the host signals it is fine to do so. But, yes, it should be possible to handle cursor_bo creation in prepare_fb without too much effort. take care, Gerd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH 08/10] drm/qxl: fix monitors object kmap
Use the correct kmap variant. We don't hold a reservation here, so we can't use the _locked variant. We can drop the pin because qxl_bo_kmap will do that for us. Signed-off-by: Gerd Hoffmann --- drivers/gpu/drm/qxl/qxl_display.c | 7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index 8672385a1caf..7500560db8e4 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -1159,12 +1159,10 @@ int qxl_create_monitors_object(struct qxl_device *qdev) } qdev->monitors_config_bo = gem_to_qxl_bo(gobj); - ret = qxl_bo_pin(qdev->monitors_config_bo); + ret = qxl_bo_kmap(qdev->monitors_config_bo, ); if (ret) return ret; - qxl_bo_kmap_locked(qdev->monitors_config_bo, ); - qdev->monitors_config = qdev->monitors_config_bo->kptr; qdev->ram_header->monitors_config = qxl_bo_physical_address(qdev, qdev->monitors_config_bo, 0); @@ -1189,8 +1187,7 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev) qdev->monitors_config = NULL; qdev->ram_header->monitors_config = 0; - qxl_bo_kunmap_locked(qdev->monitors_config_bo); - ret = qxl_bo_unpin(qdev->monitors_config_bo); + ret = qxl_bo_kunmap(qdev->monitors_config_bo); if (ret) return ret; -- 2.29.2 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH 09/10] drm/qxl: map/unmap framebuffers in prepare_fb+cleanup_fb callbacks.
We don't have to map in atomic_update callback then,
making locking a bit less complicated.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_display.c | 14 +-
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index 7500560db8e4..39b8c5116d34 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -584,7 +584,6 @@ static void qxl_cursor_atomic_update(struct drm_plane
*plane,
struct drm_gem_object *obj;
struct qxl_bo *cursor_bo = NULL, *user_bo = NULL, *old_cursor_bo = NULL;
int ret;
- struct dma_buf_map user_map;
struct dma_buf_map cursor_map;
void *user_ptr;
int size = 64*64*4;
@@ -599,11 +598,8 @@ static void qxl_cursor_atomic_update(struct drm_plane
*plane,
obj = fb->obj[0];
user_bo = gem_to_qxl_bo(obj);
- /* pinning is done in the prepare/cleanup framevbuffer */
- ret = qxl_bo_kmap_locked(user_bo, _map);
- if (ret)
- goto out_free_release;
- user_ptr = user_map.vaddr; /* TODO: Use mapping abstraction
properly */
+ /* mapping is done in the prepare/cleanup framevbuffer */
+ user_ptr = user_bo->map.vaddr; /* TODO: Use mapping abstraction
properly */
ret = qxl_alloc_bo_reserved(qdev, release,
sizeof(struct qxl_cursor) + size,
@@ -639,7 +635,6 @@ static void qxl_cursor_atomic_update(struct drm_plane
*plane,
cursor->chunk.data_size = size;
memcpy(cursor->chunk.data, user_ptr, size);
qxl_bo_kunmap_locked(cursor_bo);
- qxl_bo_kunmap_locked(user_bo);
cmd = (struct qxl_cursor_cmd *) qxl_release_map(qdev, release);
cmd->u.set.visible = 1;
@@ -778,6 +773,7 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane,
struct drm_gem_object *obj;
struct qxl_bo *user_bo;
struct qxl_surface surf;
+ struct dma_buf_map unused;
if (!new_state->fb)
return 0;
@@ -815,7 +811,7 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane,
}
}
- return qxl_bo_pin(user_bo);
+ return qxl_bo_kmap(user_bo, );
}
static void qxl_plane_cleanup_fb(struct drm_plane *plane,
@@ -834,7 +830,7 @@ static void qxl_plane_cleanup_fb(struct drm_plane *plane,
obj = old_state->fb->obj[0];
user_bo = gem_to_qxl_bo(obj);
- qxl_bo_unpin(user_bo);
+ qxl_bo_kunmap(user_bo);
if (old_state->fb != plane->state->fb && user_bo->shadow) {
qxl_bo_unpin(user_bo->shadow);
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH 07/10] drm/qxl: fix prime kmap
Use the correct kmap variant. We don't have a reservation here,
so we can't use the _locked version.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_prime.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_prime.c b/drivers/gpu/drm/qxl/qxl_prime.c
index dc295b2e2b52..4aa949799446 100644
--- a/drivers/gpu/drm/qxl/qxl_prime.c
+++ b/drivers/gpu/drm/qxl/qxl_prime.c
@@ -59,7 +59,7 @@ int qxl_gem_prime_vmap(struct drm_gem_object *obj, struct
dma_buf_map *map)
struct qxl_bo *bo = gem_to_qxl_bo(obj);
int ret;
- ret = qxl_bo_kmap_locked(bo, map);
+ ret = qxl_bo_kmap(bo, map);
if (ret < 0)
return ret;
@@ -71,7 +71,7 @@ void qxl_gem_prime_vunmap(struct drm_gem_object *obj,
{
struct qxl_bo *bo = gem_to_qxl_bo(obj);
- qxl_bo_kunmap_locked(bo);
+ qxl_bo_kunmap(bo);
}
int qxl_gem_prime_mmap(struct drm_gem_object *obj,
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH 10/10] drm/qxl: add lock asserts to qxl_bo_kmap_locked + qxl_bo_kunmap_locked
Try avoid re-introducing locking bugs.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_object.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index 22748b9566af..90d5e5b7f927 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -162,6 +162,8 @@ int qxl_bo_kmap_locked(struct qxl_bo *bo, struct
dma_buf_map *map)
{
int r;
+ dma_resv_assert_held(bo->tbo.base.resv);
+
if (bo->kptr) {
bo->map_count++;
goto out;
@@ -236,6 +238,8 @@ void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev,
void qxl_bo_kunmap_locked(struct qxl_bo *bo)
{
+ dma_resv_assert_held(bo->tbo.base.resv);
+
if (bo->kptr == NULL)
return;
bo->map_count--;
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH 05/10] drm/qxl: rename qxl_bo_kmap -> qxl_bo_kmap_locked
Make clear that these functions should be called with reserved bo's only. No functional change. Signed-off-by: Gerd Hoffmann --- drivers/gpu/drm/qxl/qxl_object.h | 4 ++-- drivers/gpu/drm/qxl/qxl_display.c | 14 +++--- drivers/gpu/drm/qxl/qxl_draw.c| 8 drivers/gpu/drm/qxl/qxl_image.c | 2 +- drivers/gpu/drm/qxl/qxl_object.c | 8 drivers/gpu/drm/qxl/qxl_prime.c | 4 ++-- 6 files changed, 20 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h index dc1659e717f1..5bd33650183f 100644 --- a/drivers/gpu/drm/qxl/qxl_object.h +++ b/drivers/gpu/drm/qxl/qxl_object.h @@ -64,8 +64,8 @@ extern int qxl_bo_create(struct qxl_device *qdev, u32 priority, struct qxl_surface *surf, struct qxl_bo **bo_ptr); -extern int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map); -extern void qxl_bo_kunmap(struct qxl_bo *bo); +extern int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map); +extern void qxl_bo_kunmap_locked(struct qxl_bo *bo); void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, int page_offset); void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, void *map); extern struct qxl_bo *qxl_bo_ref(struct qxl_bo *bo); diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index a1b5cc5918bc..8672385a1caf 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -600,7 +600,7 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, user_bo = gem_to_qxl_bo(obj); /* pinning is done in the prepare/cleanup framevbuffer */ - ret = qxl_bo_kmap(user_bo, _map); + ret = qxl_bo_kmap_locked(user_bo, _map); if (ret) goto out_free_release; user_ptr = user_map.vaddr; /* TODO: Use mapping abstraction properly */ @@ -619,7 +619,7 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, if (ret) goto out_unpin; - ret = qxl_bo_kmap(cursor_bo, _map); + ret = qxl_bo_kmap_locked(cursor_bo, _map); if (ret) goto out_backoff; if (cursor_map.is_iomem) /* TODO: Use mapping abstraction properly */ @@ -638,8 +638,8 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, cursor->chunk.prev_chunk = 0; cursor->chunk.data_size = size; memcpy(cursor->chunk.data, user_ptr, size); - qxl_bo_kunmap(cursor_bo); - qxl_bo_kunmap(user_bo); + qxl_bo_kunmap_locked(cursor_bo); + qxl_bo_kunmap_locked(user_bo); cmd = (struct qxl_cursor_cmd *) qxl_release_map(qdev, release); cmd->u.set.visible = 1; @@ -681,7 +681,7 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane, out_free_bo: qxl_bo_unref(_bo); out_kunmap: - qxl_bo_kunmap(user_bo); + qxl_bo_kunmap_locked(user_bo); out_free_release: qxl_release_free(qdev, release); return; @@ -1163,7 +1163,7 @@ int qxl_create_monitors_object(struct qxl_device *qdev) if (ret) return ret; - qxl_bo_kmap(qdev->monitors_config_bo, ); + qxl_bo_kmap_locked(qdev->monitors_config_bo, ); qdev->monitors_config = qdev->monitors_config_bo->kptr; qdev->ram_header->monitors_config = @@ -1189,7 +1189,7 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev) qdev->monitors_config = NULL; qdev->ram_header->monitors_config = 0; - qxl_bo_kunmap(qdev->monitors_config_bo); + qxl_bo_kunmap_locked(qdev->monitors_config_bo); ret = qxl_bo_unpin(qdev->monitors_config_bo); if (ret) return ret; diff --git a/drivers/gpu/drm/qxl/qxl_draw.c b/drivers/gpu/drm/qxl/qxl_draw.c index 7b7acb910780..294542d49015 100644 --- a/drivers/gpu/drm/qxl/qxl_draw.c +++ b/drivers/gpu/drm/qxl/qxl_draw.c @@ -48,7 +48,7 @@ static struct qxl_rect *drawable_set_clipping(struct qxl_device *qdev, struct qxl_clip_rects *dev_clips; int ret; - ret = qxl_bo_kmap(clips_bo, ); + ret = qxl_bo_kmap_locked(clips_bo, ); if (ret) return NULL; dev_clips = map.vaddr; /* TODO: Use mapping abstraction properly */ @@ -202,7 +202,7 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev, if (ret) goto out_release_backoff; - ret = qxl_bo_kmap(bo, _map); + ret = qxl_bo_kmap_locked(bo, _map); if (ret) goto out_release_backoff; surface_base = surface_map.vaddr; /* TODO: Use mapping abstraction properly */ @@ -210,7 +210,7 @@ void qxl_draw_dirty_fb(struct
[Spice-devel] [PATCH 06/10] drm/qxl: add qxl_bo_kmap/qxl_bo_kunmap
Add kmap/kunmap variants which reserve (and pin) the bo.
They can be used in case the caller doesn't hold a reservation
for the bo.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_object.h | 2 ++
drivers/gpu/drm/qxl/qxl_object.c | 36
2 files changed, 38 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h
index 5bd33650183f..360972ae4869 100644
--- a/drivers/gpu/drm/qxl/qxl_object.h
+++ b/drivers/gpu/drm/qxl/qxl_object.h
@@ -64,7 +64,9 @@ extern int qxl_bo_create(struct qxl_device *qdev,
u32 priority,
struct qxl_surface *surf,
struct qxl_bo **bo_ptr);
+extern int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map);
extern int qxl_bo_kmap_locked(struct qxl_bo *bo, struct dma_buf_map *map);
+extern int qxl_bo_kunmap(struct qxl_bo *bo);
extern void qxl_bo_kunmap_locked(struct qxl_bo *bo);
void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo, int
page_offset);
void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev, struct qxl_bo *bo,
void *map);
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index b56d4dfc28cb..22748b9566af 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -29,6 +29,9 @@
#include "qxl_drv.h"
#include "qxl_object.h"
+static int __qxl_bo_pin(struct qxl_bo *bo);
+static void __qxl_bo_unpin(struct qxl_bo *bo);
+
static void qxl_ttm_bo_destroy(struct ttm_buffer_object *tbo)
{
struct qxl_bo *bo;
@@ -179,6 +182,25 @@ int qxl_bo_kmap_locked(struct qxl_bo *bo, struct
dma_buf_map *map)
return 0;
}
+int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map)
+{
+ int r;
+
+ r = qxl_bo_reserve(bo);
+ if (r)
+ return r;
+
+ r = __qxl_bo_pin(bo);
+ if (r) {
+ qxl_bo_unreserve(bo);
+ return r;
+ }
+
+ r = qxl_bo_kmap_locked(bo, map);
+ qxl_bo_unreserve(bo);
+ return r;
+}
+
void *qxl_bo_kmap_atomic_page(struct qxl_device *qdev,
struct qxl_bo *bo, int page_offset)
{
@@ -223,6 +245,20 @@ void qxl_bo_kunmap_locked(struct qxl_bo *bo)
ttm_bo_vunmap(>tbo, >map);
}
+int qxl_bo_kunmap(struct qxl_bo *bo)
+{
+ int r;
+
+ r = qxl_bo_reserve(bo);
+ if (r)
+ return r;
+
+ qxl_bo_kunmap_locked(bo);
+ __qxl_bo_unpin(bo);
+ qxl_bo_unreserve(bo);
+ return 0;
+}
+
void qxl_bo_kunmap_atomic_page(struct qxl_device *qdev,
struct qxl_bo *bo, void *pmap)
{
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH 04/10] drm/qxl: fix lockdep issue in qxl_alloc_release_reserved
Call qxl_bo_unpin (which does a reservation) without holding the
release_mutex lock. Fixes lockdep (correctly) warning on a possible
deadlock.
Fixes: 65ffea3c6e73 ("drm/qxl: unpin release objects")
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_release.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_release.c
b/drivers/gpu/drm/qxl/qxl_release.c
index a831184e014a..352a11a8485b 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -284,7 +284,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev,
unsigned long size,
int type, struct qxl_release **release,
struct qxl_bo **rbo)
{
- struct qxl_bo *bo;
+ struct qxl_bo *bo, *free_bo = NULL;
int idr_ret;
int ret = 0;
union qxl_release_info *info;
@@ -317,8 +317,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev,
unsigned long size,
mutex_lock(>release_mutex);
if (qdev->current_release_bo_offset[cur_idx] + 1 >=
releases_per_bo[cur_idx]) {
- qxl_bo_unpin(qdev->current_release_bo[cur_idx]);
- qxl_bo_unref(>current_release_bo[cur_idx]);
+ free_bo = qdev->current_release_bo[cur_idx];
qdev->current_release_bo_offset[cur_idx] = 0;
qdev->current_release_bo[cur_idx] = NULL;
}
@@ -326,6 +325,10 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev,
unsigned long size,
ret = qxl_release_bo_alloc(qdev,
>current_release_bo[cur_idx], priority);
if (ret) {
mutex_unlock(>release_mutex);
+ if (free_bo) {
+ qxl_bo_unpin(free_bo);
+ qxl_bo_unref(_bo);
+ }
qxl_release_free(qdev, *release);
return ret;
}
@@ -341,6 +344,10 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev,
unsigned long size,
*rbo = bo;
mutex_unlock(>release_mutex);
+ if (free_bo) {
+ qxl_bo_unpin(free_bo);
+ qxl_bo_unref(_bo);
+ }
ret = qxl_release_list_add(*release, bo);
qxl_bo_unref();
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH 01/10] drm/qxl: properly handle device init failures
Specifically do not try release resources which where
not allocated in the first place.
Cc: Tong Zhang
Tested-by: Tong Zhang
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_display.c | 3 +++
drivers/gpu/drm/qxl/qxl_kms.c | 4
2 files changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index c326412136c5..ec50d2cfd4e1 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -1183,6 +1183,9 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev)
{
int ret;
+ if (!qdev->monitors_config_bo)
+ return 0;
+
qdev->monitors_config = NULL;
qdev->ram_header->monitors_config = 0;
diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index 66d74aaaee06..4dc5ad13f12c 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -288,6 +288,10 @@ void qxl_device_fini(struct qxl_device *qdev)
{
int cur_idx;
+ /* check if qxl_device_init() was successful (gc_work is initialized
last) */
+ if (!qdev->gc_work.func)
+ return;
+
for (cur_idx = 0; cur_idx < 3; cur_idx++) {
if (!qdev->current_release_bo[cur_idx])
continue;
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH 03/10] drm/qxl: use ttm bo priorities
Allow to set priorities for buffer objects. Use priority 1 for surface
and cursor command releases. Use priority 0 for drawing command
releases. That way the short-living drawing commands are first in line
when it comes to eviction, making it *much* less likely that
ttm_bo_mem_force_space() picks something which can't be evicted and
throws an error after waiting a while without success.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_object.h | 1 +
drivers/gpu/drm/qxl/qxl_cmd.c | 2 +-
drivers/gpu/drm/qxl/qxl_display.c | 4 ++--
drivers/gpu/drm/qxl/qxl_gem.c | 2 +-
drivers/gpu/drm/qxl/qxl_object.c | 5 +++--
drivers/gpu/drm/qxl/qxl_release.c | 19 +--
6 files changed, 21 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h
index e60a8f88e226..dc1659e717f1 100644
--- a/drivers/gpu/drm/qxl/qxl_object.h
+++ b/drivers/gpu/drm/qxl/qxl_object.h
@@ -61,6 +61,7 @@ static inline u64 qxl_bo_mmap_offset(struct qxl_bo *bo)
extern int qxl_bo_create(struct qxl_device *qdev,
unsigned long size,
bool kernel, bool pinned, u32 domain,
+u32 priority,
struct qxl_surface *surf,
struct qxl_bo **bo_ptr);
extern int qxl_bo_kmap(struct qxl_bo *bo, struct dma_buf_map *map);
diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index 7e22a81bfb36..7b00c955cd82 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -269,7 +269,7 @@ int qxl_alloc_bo_reserved(struct qxl_device *qdev,
int ret;
ret = qxl_bo_create(qdev, size, false /* not kernel - device */,
- false, QXL_GEM_DOMAIN_VRAM, NULL, );
+ false, QXL_GEM_DOMAIN_VRAM, 0, NULL, );
if (ret) {
DRM_ERROR("failed to allocate VRAM BO\n");
return ret;
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index ec50d2cfd4e1..a1b5cc5918bc 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -799,8 +799,8 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane,
qdev->dumb_shadow_bo = NULL;
}
qxl_bo_create(qdev, surf.height * surf.stride,
- true, true, QXL_GEM_DOMAIN_SURFACE, ,
- >dumb_shadow_bo);
+ true, true, QXL_GEM_DOMAIN_SURFACE, 0,
+ , >dumb_shadow_bo);
}
if (user_bo->shadow != qdev->dumb_shadow_bo) {
if (user_bo->shadow) {
diff --git a/drivers/gpu/drm/qxl/qxl_gem.c b/drivers/gpu/drm/qxl/qxl_gem.c
index 48e096285b4c..a08da0bd9098 100644
--- a/drivers/gpu/drm/qxl/qxl_gem.c
+++ b/drivers/gpu/drm/qxl/qxl_gem.c
@@ -55,7 +55,7 @@ int qxl_gem_object_create(struct qxl_device *qdev, int size,
/* At least align on page size */
if (alignment < PAGE_SIZE)
alignment = PAGE_SIZE;
- r = qxl_bo_create(qdev, size, kernel, false, initial_domain, surf,
);
+ r = qxl_bo_create(qdev, size, kernel, false, initial_domain, 0, surf,
);
if (r) {
if (r != -ERESTARTSYS)
DRM_ERROR(
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index 705b51535492..7eada4ad217b 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -103,8 +103,8 @@ static const struct drm_gem_object_funcs qxl_object_funcs =
{
.print_info = drm_gem_ttm_print_info,
};
-int qxl_bo_create(struct qxl_device *qdev,
- unsigned long size, bool kernel, bool pinned, u32 domain,
+int qxl_bo_create(struct qxl_device *qdev, unsigned long size,
+ bool kernel, bool pinned, u32 domain, u32 priority,
struct qxl_surface *surf,
struct qxl_bo **bo_ptr)
{
@@ -137,6 +137,7 @@ int qxl_bo_create(struct qxl_device *qdev,
qxl_ttm_placement_from_domain(bo, domain);
+ bo->tbo.priority = priority;
r = ttm_bo_init_reserved(>mman.bdev, >tbo, size, type,
>placement, 0, , NULL, NULL,
_ttm_bo_destroy);
diff --git a/drivers/gpu/drm/qxl/qxl_release.c
b/drivers/gpu/drm/qxl/qxl_release.c
index 63818b56218c..a831184e014a 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -161,11 +161,12 @@ qxl_release_free(struct qxl_device *qdev,
}
static int qxl_release_bo_alloc(struct qxl_device *qdev,
- struct qxl_bo **bo)
+ struct qxl_bo **bo,
+ u32 priority)
{
[Spice-devel] [PATCH 02/10] drm/qxl: more fence wait rework
Move qxl_io_notify_oom() call into wait condition.
That way the driver will call it again if one call
wasn't enough.
Also allows to remove the extra dma_fence_is_signaled()
check and the goto.
Fixes: 5a838e5d5825 ("drm/qxl: simplify qxl_fence_wait")
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_release.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_release.c
b/drivers/gpu/drm/qxl/qxl_release.c
index 6ed673d75f9f..63818b56218c 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -62,16 +62,13 @@ static long qxl_fence_wait(struct dma_fence *fence, bool
intr,
qdev = container_of(fence->lock, struct qxl_device, release_lock);
- if (dma_fence_is_signaled(fence))
- goto signaled;
-
- qxl_io_notify_oom(qdev);
if (!wait_event_timeout(qdev->release_event,
- dma_fence_is_signaled(fence),
+ (dma_fence_is_signaled(fence) || (
+ qxl_io_notify_oom(qdev),
+ 0)),
timeout))
return 0;
-signaled:
cur = jiffies;
if (time_after(cur, end))
return 0;
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] drm/qxl: properly handle device init failures
On Mon, Feb 08, 2021 at 12:07:01PM -0500, Tong Zhang wrote: > Does this patch fix an issue raised previously? Or should they be used > together? > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2466541.html > > IMHO using this patch alone won’t fix the issue This patch on top of drm-misc-next fixes the initialization error issue reported by you in my testing. take care, Gerd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH] drm/qxl: properly handle device init failures
Specifically do not try release resources which where
not allocated in the first place.
Cc: Tong Zhang
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_display.c | 3 +++
drivers/gpu/drm/qxl/qxl_kms.c | 4
2 files changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index c326412136c5..ec50d2cfd4e1 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -1183,6 +1183,9 @@ int qxl_destroy_monitors_object(struct qxl_device *qdev)
{
int ret;
+ if (!qdev->monitors_config_bo)
+ return 0;
+
qdev->monitors_config = NULL;
qdev->ram_header->monitors_config = 0;
diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index 66d74aaaee06..4dc5ad13f12c 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -288,6 +288,10 @@ void qxl_device_fini(struct qxl_device *qdev)
{
int cur_idx;
+ /* check if qxl_device_init() was successful (gc_work is initialized
last) */
+ if (!qdev->gc_work.func)
+ return;
+
for (cur_idx = 0; cur_idx < 3; cur_idx++) {
if (!qdev->current_release_bo[cur_idx])
continue;
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v2] drm/qxl: do not run release if qxl failed to init
On Thu, Feb 04, 2021 at 11:30:50AM -0500, Tong Zhang wrote: > if qxl_device_init() fail, drm device will not be registered, > in this case, do not run qxl_drm_release() How do you trigger this? take care, Gerd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v6 03/10] drm/qxl: use drmm_mode_config_init
Signed-off-by: Gerd Hoffmann
Reviewed-by: Daniel Vetter
Acked-by: Thomas Zimmermann
---
drivers/gpu/drm/qxl/qxl_display.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index 012bce0cdb65..38d6b596094d 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -1195,7 +1195,9 @@ int qxl_modeset_init(struct qxl_device *qdev)
int i;
int ret;
- drm_mode_config_init(>ddev);
+ ret = drmm_mode_config_init(>ddev);
+ if (ret)
+ return ret;
ret = qxl_create_monitors_object(qdev);
if (ret)
@@ -1228,5 +1230,4 @@ int qxl_modeset_init(struct qxl_device *qdev)
void qxl_modeset_fini(struct qxl_device *qdev)
{
qxl_destroy_monitors_object(qdev);
- drm_mode_config_cleanup(>ddev);
}
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v6 10/10] drm/qxl: allocate dumb buffers in ram
dumb buffers are shadowed anyway, so there is no need to store them in device memory. Use QXL_GEM_DOMAIN_CPU (TTM_PL_SYSTEM) instead. Signed-off-by: Gerd Hoffmann --- drivers/gpu/drm/qxl/qxl_dumb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/qxl/qxl_dumb.c b/drivers/gpu/drm/qxl/qxl_dumb.c index c04cd5a2553c..48a58ba1db96 100644 --- a/drivers/gpu/drm/qxl/qxl_dumb.c +++ b/drivers/gpu/drm/qxl/qxl_dumb.c @@ -59,7 +59,7 @@ int qxl_mode_dumb_create(struct drm_file *file_priv, surf.stride = pitch; surf.format = format; r = qxl_gem_object_create_with_handle(qdev, file_priv, - QXL_GEM_DOMAIN_SURFACE, + QXL_GEM_DOMAIN_CPU, args->size, , , ); if (r) -- 2.29.2 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v6 08/10] drm/qxl: properly free qxl releases
Reorganize qxl_device_fini() a bit.
Add missing unpin() calls.
Count releases. Add wait queue for releases. That way
qxl_device_fini() can easily wait until everything is
ready for proper shutdown.
Signed-off-by: Gerd Hoffmann
Acked-by: Thomas Zimmermann
---
drivers/gpu/drm/qxl/qxl_drv.h | 2 ++
drivers/gpu/drm/qxl/qxl_cmd.c | 1 +
drivers/gpu/drm/qxl/qxl_irq.c | 1 +
drivers/gpu/drm/qxl/qxl_kms.c | 28
drivers/gpu/drm/qxl/qxl_release.c | 2 ++
5 files changed, 30 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 01354b43c413..6dd57cfb2e7c 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -214,6 +214,8 @@ struct qxl_device {
spinlock_t release_lock;
struct idr release_idr;
uint32_trelease_seqno;
+ atomic_trelease_count;
+ wait_queue_head_t release_event;
spinlock_t release_idr_lock;
struct mutexasync_io_mutex;
unsigned int last_sent_io_cmd;
diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index 54e3c3a97440..7e22a81bfb36 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -254,6 +254,7 @@ int qxl_garbage_collect(struct qxl_device *qdev)
}
}
+ wake_up_all(>release_event);
DRM_DEBUG_DRIVER("%d\n", i);
return i;
diff --git a/drivers/gpu/drm/qxl/qxl_irq.c b/drivers/gpu/drm/qxl/qxl_irq.c
index ddf6588a2a38..d312322cacd1 100644
--- a/drivers/gpu/drm/qxl/qxl_irq.c
+++ b/drivers/gpu/drm/qxl/qxl_irq.c
@@ -87,6 +87,7 @@ int qxl_irq_init(struct qxl_device *qdev)
init_waitqueue_head(>display_event);
init_waitqueue_head(>cursor_event);
init_waitqueue_head(>io_cmd_event);
+ init_waitqueue_head(>release_event);
INIT_WORK(>client_monitors_config_work,
qxl_client_monitors_config_work_func);
atomic_set(>irq_received, 0);
diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index 4a60a52ab62e..66d74aaaee06 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -286,11 +286,31 @@ int qxl_device_init(struct qxl_device *qdev,
void qxl_device_fini(struct qxl_device *qdev)
{
- qxl_bo_unref(>current_release_bo[0]);
- qxl_bo_unref(>current_release_bo[1]);
- qxl_gem_fini(qdev);
- qxl_bo_fini(qdev);
+ int cur_idx;
+
+ for (cur_idx = 0; cur_idx < 3; cur_idx++) {
+ if (!qdev->current_release_bo[cur_idx])
+ continue;
+ qxl_bo_unpin(qdev->current_release_bo[cur_idx]);
+ qxl_bo_unref(>current_release_bo[cur_idx]);
+ qdev->current_release_bo_offset[cur_idx] = 0;
+ qdev->current_release_bo[cur_idx] = NULL;
+ }
+
+ /*
+* Ask host to release resources (+fill release ring),
+* then wait for the release actually happening.
+*/
+ qxl_io_notify_oom(qdev);
+ wait_event_timeout(qdev->release_event,
+ atomic_read(>release_count) == 0,
+ HZ);
flush_work(>gc_work);
+ qxl_surf_evict(qdev);
+ qxl_vram_evict(qdev);
+
+ qxl_gem_fini(qdev);
+ qxl_bo_fini(qdev);
qxl_ring_free(qdev->command_ring);
qxl_ring_free(qdev->cursor_ring);
qxl_ring_free(qdev->release_ring);
diff --git a/drivers/gpu/drm/qxl/qxl_release.c
b/drivers/gpu/drm/qxl/qxl_release.c
index 28013fd1f8ea..43a5436853b7 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -196,6 +196,7 @@ qxl_release_free(struct qxl_device *qdev,
qxl_release_free_list(release);
kfree(release);
}
+ atomic_dec(>release_count);
}
static int qxl_release_bo_alloc(struct qxl_device *qdev,
@@ -344,6 +345,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev,
unsigned long size,
*rbo = NULL;
return idr_ret;
}
+ atomic_inc(>release_count);
mutex_lock(>release_mutex);
if (qdev->current_release_bo_offset[cur_idx] + 1 >=
releases_per_bo[cur_idx]) {
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v6 02/10] Revert "drm/qxl: do not run release if qxl failed to init"
This reverts commit b91907a6241193465ca92e357adf16822242296d. Patch is broken, it effectively makes qxl_drm_release() a nop because on normal driver shutdown qxl_drm_release() is called *after* drm_dev_unregister(). Cc: Tong Zhang Signed-off-by: Gerd Hoffmann --- drivers/gpu/drm/qxl/qxl_drv.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c index 34c8b25b5780..fb5f6a5e81d7 100644 --- a/drivers/gpu/drm/qxl/qxl_drv.c +++ b/drivers/gpu/drm/qxl/qxl_drv.c @@ -144,8 +144,6 @@ static void qxl_drm_release(struct drm_device *dev) * reodering qxl_modeset_fini() + qxl_device_fini() calls is * non-trivial though. */ - if (!dev->registered) - return; qxl_modeset_fini(qdev); qxl_device_fini(qdev); } -- 2.29.2 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v6 05/10] drm/qxl: release shadow on shutdown
In case we have a shadow surface on shutdown release
it so it doesn't leak.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_display.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index 38d6b596094d..60331e31861a 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -1229,5 +1229,9 @@ int qxl_modeset_init(struct qxl_device *qdev)
void qxl_modeset_fini(struct qxl_device *qdev)
{
+ if (qdev->dumb_shadow_bo) {
+ drm_gem_object_put(>dumb_shadow_bo->tbo.base);
+ qdev->dumb_shadow_bo = NULL;
+ }
qxl_destroy_monitors_object(qdev);
}
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v6 06/10] drm/qxl: properly pin/unpin shadow
Suggested-by: Thomas Zimmermann
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_display.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index 60331e31861a..d25fd3acc891 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -802,12 +802,14 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane,
}
if (user_bo->shadow != qdev->dumb_shadow_bo) {
if (user_bo->shadow) {
+ qxl_bo_unpin(user_bo->shadow);
drm_gem_object_put
(_bo->shadow->tbo.base);
user_bo->shadow = NULL;
}
drm_gem_object_get(>dumb_shadow_bo->tbo.base);
user_bo->shadow = qdev->dumb_shadow_bo;
+ qxl_bo_pin(user_bo->shadow);
}
}
@@ -833,6 +835,7 @@ static void qxl_plane_cleanup_fb(struct drm_plane *plane,
qxl_bo_unpin(user_bo);
if (old_state->fb != plane->state->fb && user_bo->shadow) {
+ qxl_bo_unpin(user_bo->shadow);
drm_gem_object_put(_bo->shadow->tbo.base);
user_bo->shadow = NULL;
}
@@ -1230,6 +1233,7 @@ int qxl_modeset_init(struct qxl_device *qdev)
void qxl_modeset_fini(struct qxl_device *qdev)
{
if (qdev->dumb_shadow_bo) {
+ qxl_bo_unpin(qdev->dumb_shadow_bo);
drm_gem_object_put(>dumb_shadow_bo->tbo.base);
qdev->dumb_shadow_bo = NULL;
}
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v6 07/10] drm/qxl: handle shadow in primary destroy
qxl_primary_atomic_disable must check whenever the framebuffer bo has a
shadow surface and in case it has check the shadow primary status.
Signed-off-by: Gerd Hoffmann
Acked-by: Thomas Zimmermann
---
drivers/gpu/drm/qxl/qxl_display.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index d25fd3acc891..c326412136c5 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -562,6 +562,8 @@ static void qxl_primary_atomic_disable(struct drm_plane
*plane,
if (old_state->fb) {
struct qxl_bo *bo = gem_to_qxl_bo(old_state->fb->obj[0]);
+ if (bo->shadow)
+ bo = bo->shadow;
if (bo->is_primary) {
qxl_io_destroy_primary(qdev);
bo->is_primary = false;
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v6 09/10] drm/qxl: simplify qxl_fence_wait
Now that we have the new release_event wait queue we can just
use that in qxl_fence_wait() and simplify the code alot.
Signed-off-by: Gerd Hoffmann
Acked-by: Thomas Zimmermann
---
drivers/gpu/drm/qxl/qxl_release.c | 44 +++
1 file changed, 4 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_release.c
b/drivers/gpu/drm/qxl/qxl_release.c
index 43a5436853b7..6ed673d75f9f 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -58,54 +58,18 @@ static long qxl_fence_wait(struct dma_fence *fence, bool
intr,
signed long timeout)
{
struct qxl_device *qdev;
- struct qxl_release *release;
- int count = 0, sc = 0;
- bool have_drawable_releases;
unsigned long cur, end = jiffies + timeout;
qdev = container_of(fence->lock, struct qxl_device, release_lock);
- release = container_of(fence, struct qxl_release, base);
- have_drawable_releases = release->type == QXL_RELEASE_DRAWABLE;
-
-retry:
- sc++;
if (dma_fence_is_signaled(fence))
goto signaled;
qxl_io_notify_oom(qdev);
-
- for (count = 0; count < 11; count++) {
- if (!qxl_queue_garbage_collect(qdev, true))
- break;
-
- if (dma_fence_is_signaled(fence))
- goto signaled;
- }
-
- if (dma_fence_is_signaled(fence))
- goto signaled;
-
- if (have_drawable_releases || sc < 4) {
- if (sc > 2)
- /* back off */
- usleep_range(500, 1000);
-
- if (time_after(jiffies, end))
- return 0;
-
- if (have_drawable_releases && sc > 300) {
- DMA_FENCE_WARN(fence, "failed to wait on release %llu "
- "after spincount %d\n",
- fence->context & ~0xf000, sc);
- goto signaled;
- }
- goto retry;
- }
- /*
-* yeah, original sync_obj_wait gave up after 3 spins when
-* have_drawable_releases is not set.
-*/
+ if (!wait_event_timeout(qdev->release_event,
+ dma_fence_is_signaled(fence),
+ timeout))
+ return 0;
signaled:
cur = jiffies;
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v6 04/10] drm/qxl: unpin release objects
Balances the qxl_create_bo(..., pinned=true, ...);
call in qxl_release_bo_alloc().
Signed-off-by: Gerd Hoffmann
Acked-by: Thomas Zimmermann
---
drivers/gpu/drm/qxl/qxl_release.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/qxl/qxl_release.c
b/drivers/gpu/drm/qxl/qxl_release.c
index c52412724c26..28013fd1f8ea 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -347,6 +347,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev,
unsigned long size,
mutex_lock(>release_mutex);
if (qdev->current_release_bo_offset[cur_idx] + 1 >=
releases_per_bo[cur_idx]) {
+ qxl_bo_unpin(qdev->current_release_bo[cur_idx]);
qxl_bo_unref(>current_release_bo[cur_idx]);
qdev->current_release_bo_offset[cur_idx] = 0;
qdev->current_release_bo[cur_idx] = NULL;
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v5 1/6] drm/qxl: use drmm_mode_config_init
Signed-off-by: Gerd Hoffmann
Reviewed-by: Daniel Vetter
Acked-by: Thomas Zimmermann
---
drivers/gpu/drm/qxl/qxl_display.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index 012bce0cdb65..38d6b596094d 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -1195,7 +1195,9 @@ int qxl_modeset_init(struct qxl_device *qdev)
int i;
int ret;
- drm_mode_config_init(>ddev);
+ ret = drmm_mode_config_init(>ddev);
+ if (ret)
+ return ret;
ret = qxl_create_monitors_object(qdev);
if (ret)
@@ -1228,5 +1230,4 @@ int qxl_modeset_init(struct qxl_device *qdev)
void qxl_modeset_fini(struct qxl_device *qdev)
{
qxl_destroy_monitors_object(qdev);
- drm_mode_config_cleanup(>ddev);
}
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v5 5/6] drm/qxl: properly free qxl releases
Reorganize qxl_device_fini() a bit.
Add missing unpin() calls.
Count releases. Add wait queue for releases. That way
qxl_device_fini() can easily wait until everything is
ready for proper shutdown.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_drv.h | 2 ++
drivers/gpu/drm/qxl/qxl_cmd.c | 1 +
drivers/gpu/drm/qxl/qxl_irq.c | 1 +
drivers/gpu/drm/qxl/qxl_kms.c | 22 --
drivers/gpu/drm/qxl/qxl_release.c | 2 ++
5 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 01354b43c413..6dd57cfb2e7c 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -214,6 +214,8 @@ struct qxl_device {
spinlock_t release_lock;
struct idr release_idr;
uint32_trelease_seqno;
+ atomic_trelease_count;
+ wait_queue_head_t release_event;
spinlock_t release_idr_lock;
struct mutexasync_io_mutex;
unsigned int last_sent_io_cmd;
diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index 54e3c3a97440..7e22a81bfb36 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -254,6 +254,7 @@ int qxl_garbage_collect(struct qxl_device *qdev)
}
}
+ wake_up_all(>release_event);
DRM_DEBUG_DRIVER("%d\n", i);
return i;
diff --git a/drivers/gpu/drm/qxl/qxl_irq.c b/drivers/gpu/drm/qxl/qxl_irq.c
index ddf6588a2a38..d312322cacd1 100644
--- a/drivers/gpu/drm/qxl/qxl_irq.c
+++ b/drivers/gpu/drm/qxl/qxl_irq.c
@@ -87,6 +87,7 @@ int qxl_irq_init(struct qxl_device *qdev)
init_waitqueue_head(>display_event);
init_waitqueue_head(>cursor_event);
init_waitqueue_head(>io_cmd_event);
+ init_waitqueue_head(>release_event);
INIT_WORK(>client_monitors_config_work,
qxl_client_monitors_config_work_func);
atomic_set(>irq_received, 0);
diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index 4a60a52ab62e..616aea948863 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -286,8 +286,26 @@ int qxl_device_init(struct qxl_device *qdev,
void qxl_device_fini(struct qxl_device *qdev)
{
- qxl_bo_unref(>current_release_bo[0]);
- qxl_bo_unref(>current_release_bo[1]);
+ int cur_idx;
+
+ for (cur_idx = 0; cur_idx < 3; cur_idx++) {
+ if (!qdev->current_release_bo[cur_idx])
+ continue;
+ qxl_bo_unpin(qdev->current_release_bo[cur_idx]);
+ qxl_bo_unref(>current_release_bo[cur_idx]);
+ qdev->current_release_bo_offset[cur_idx] = 0;
+ qdev->current_release_bo[cur_idx] = NULL;
+ }
+
+ /*
+* Ask host to release resources (+fill release ring),
+* then wait for the release actually happening.
+*/
+ qxl_io_notify_oom(qdev);
+ wait_event_timeout(qdev->release_event,
+ atomic_read(>release_count) == 0,
+ HZ);
+
qxl_gem_fini(qdev);
qxl_bo_fini(qdev);
flush_work(>gc_work);
diff --git a/drivers/gpu/drm/qxl/qxl_release.c
b/drivers/gpu/drm/qxl/qxl_release.c
index 28013fd1f8ea..43a5436853b7 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -196,6 +196,7 @@ qxl_release_free(struct qxl_device *qdev,
qxl_release_free_list(release);
kfree(release);
}
+ atomic_dec(>release_count);
}
static int qxl_release_bo_alloc(struct qxl_device *qdev,
@@ -344,6 +345,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev,
unsigned long size,
*rbo = NULL;
return idr_ret;
}
+ atomic_inc(>release_count);
mutex_lock(>release_mutex);
if (qdev->current_release_bo_offset[cur_idx] + 1 >=
releases_per_bo[cur_idx]) {
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v5 6/6] drm/qxl: simplify qxl_fence_wait
Now that we have the new release_event wait queue we can just
use that in qxl_fence_wait() and simplify the code alot.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_release.c | 42 +++
1 file changed, 4 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_release.c
b/drivers/gpu/drm/qxl/qxl_release.c
index 43a5436853b7..b6f4b8dcf228 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -59,53 +59,19 @@ static long qxl_fence_wait(struct dma_fence *fence, bool
intr,
{
struct qxl_device *qdev;
struct qxl_release *release;
- int count = 0, sc = 0;
- bool have_drawable_releases;
unsigned long cur, end = jiffies + timeout;
qdev = container_of(fence->lock, struct qxl_device, release_lock);
release = container_of(fence, struct qxl_release, base);
- have_drawable_releases = release->type == QXL_RELEASE_DRAWABLE;
-
-retry:
- sc++;
if (dma_fence_is_signaled(fence))
goto signaled;
qxl_io_notify_oom(qdev);
-
- for (count = 0; count < 11; count++) {
- if (!qxl_queue_garbage_collect(qdev, true))
- break;
-
- if (dma_fence_is_signaled(fence))
- goto signaled;
- }
-
- if (dma_fence_is_signaled(fence))
- goto signaled;
-
- if (have_drawable_releases || sc < 4) {
- if (sc > 2)
- /* back off */
- usleep_range(500, 1000);
-
- if (time_after(jiffies, end))
- return 0;
-
- if (have_drawable_releases && sc > 300) {
- DMA_FENCE_WARN(fence, "failed to wait on release %llu "
- "after spincount %d\n",
- fence->context & ~0xf000, sc);
- goto signaled;
- }
- goto retry;
- }
- /*
-* yeah, original sync_obj_wait gave up after 3 spins when
-* have_drawable_releases is not set.
-*/
+ if (!wait_event_timeout(qdev->release_event,
+ dma_fence_is_signaled(fence),
+ timeout))
+ return 0;
signaled:
cur = jiffies;
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v5 2/6] drm/qxl: unpin release objects
Balances the qxl_create_bo(..., pinned=true, ...);
call in qxl_release_bo_alloc().
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_release.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/qxl/qxl_release.c
b/drivers/gpu/drm/qxl/qxl_release.c
index c52412724c26..28013fd1f8ea 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -347,6 +347,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev,
unsigned long size,
mutex_lock(>release_mutex);
if (qdev->current_release_bo_offset[cur_idx] + 1 >=
releases_per_bo[cur_idx]) {
+ qxl_bo_unpin(qdev->current_release_bo[cur_idx]);
qxl_bo_unref(>current_release_bo[cur_idx]);
qdev->current_release_bo_offset[cur_idx] = 0;
qdev->current_release_bo[cur_idx] = NULL;
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v5 4/6] drm/qxl: handle shadow in primary destroy
qxl_primary_atomic_disable must check whenever the framebuffer bo has a
shadow surface and in case it has check the shadow primary status.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_display.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index 60331e31861a..f5ee8cd72b5b 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -562,6 +562,8 @@ static void qxl_primary_atomic_disable(struct drm_plane
*plane,
if (old_state->fb) {
struct qxl_bo *bo = gem_to_qxl_bo(old_state->fb->obj[0]);
+ if (bo->shadow)
+ bo = bo->shadow;
if (bo->is_primary) {
qxl_io_destroy_primary(qdev);
bo->is_primary = false;
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v5 3/6] drm/qxl: release shadow on shutdown
In case we have a shadow surface on shutdown release
it so it doesn't leak.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_display.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index 38d6b596094d..60331e31861a 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -1229,5 +1229,9 @@ int qxl_modeset_init(struct qxl_device *qdev)
void qxl_modeset_fini(struct qxl_device *qdev)
{
+ if (qdev->dumb_shadow_bo) {
+ drm_gem_object_put(>dumb_shadow_bo->tbo.base);
+ qdev->dumb_shadow_bo = NULL;
+ }
qxl_destroy_monitors_object(qdev);
}
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v4 5/5] drm/qxl: properly free qxl releases
> > + /* > > +* Ask host to release resources (+fill release ring), > > +* then wait for the release actually happening. > > +*/ > > + qxl_io_notify_oom(qdev); > > + for (try = 0; try < 20 && atomic_read(>release_count) > 0; try++) > > + msleep(20); > > A bit icky, why not use a wait queue or something like that instead of > hand-rolling this? Not for perf reasons, just so it's a bit clear who > waits for whom and why. There isn't one ready for use, and adding a new one (to wait for the garbage collection having released something) just for a clean shutdown looked a bit like overkill. But after digging a bit more and looking at the qxl_fence_wait() mess I think adding a wait queue looks like a good idea ... v5 coming soon ... take care, Gerd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v4 1/5] drm/qxl: use drmm_mode_config_init
Signed-off-by: Gerd Hoffmann
Reviewed-by: Daniel Vetter
Acked-by: Thomas Zimmermann
---
drivers/gpu/drm/qxl/qxl_display.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index 012bce0cdb65..38d6b596094d 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -1195,7 +1195,9 @@ int qxl_modeset_init(struct qxl_device *qdev)
int i;
int ret;
- drm_mode_config_init(>ddev);
+ ret = drmm_mode_config_init(>ddev);
+ if (ret)
+ return ret;
ret = qxl_create_monitors_object(qdev);
if (ret)
@@ -1228,5 +1230,4 @@ int qxl_modeset_init(struct qxl_device *qdev)
void qxl_modeset_fini(struct qxl_device *qdev)
{
qxl_destroy_monitors_object(qdev);
- drm_mode_config_cleanup(>ddev);
}
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v4 2/5] drm/qxl: unpin release objects
Balances the qxl_create_bo(..., pinned=true, ...);
call in qxl_release_bo_alloc().
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_release.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/qxl/qxl_release.c
b/drivers/gpu/drm/qxl/qxl_release.c
index c52412724c26..28013fd1f8ea 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -347,6 +347,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev,
unsigned long size,
mutex_lock(>release_mutex);
if (qdev->current_release_bo_offset[cur_idx] + 1 >=
releases_per_bo[cur_idx]) {
+ qxl_bo_unpin(qdev->current_release_bo[cur_idx]);
qxl_bo_unref(>current_release_bo[cur_idx]);
qdev->current_release_bo_offset[cur_idx] = 0;
qdev->current_release_bo[cur_idx] = NULL;
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v4 5/5] drm/qxl: properly free qxl releases
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_drv.h | 1 +
drivers/gpu/drm/qxl/qxl_kms.c | 22 --
drivers/gpu/drm/qxl/qxl_release.c | 2 ++
3 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 01354b43c413..1c57b587b6a7 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -214,6 +214,7 @@ struct qxl_device {
spinlock_t release_lock;
struct idr release_idr;
uint32_trelease_seqno;
+ atomic_trelease_count;
spinlock_t release_idr_lock;
struct mutexasync_io_mutex;
unsigned int last_sent_io_cmd;
diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index 4a60a52ab62e..f177f72bfc12 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -25,6 +25,7 @@
#include
#include
+#include
#include
#include
@@ -286,8 +287,25 @@ int qxl_device_init(struct qxl_device *qdev,
void qxl_device_fini(struct qxl_device *qdev)
{
- qxl_bo_unref(>current_release_bo[0]);
- qxl_bo_unref(>current_release_bo[1]);
+ int cur_idx, try;
+
+ for (cur_idx = 0; cur_idx < 3; cur_idx++) {
+ if (!qdev->current_release_bo[cur_idx])
+ continue;
+ qxl_bo_unpin(qdev->current_release_bo[cur_idx]);
+ qxl_bo_unref(>current_release_bo[cur_idx]);
+ qdev->current_release_bo_offset[cur_idx] = 0;
+ qdev->current_release_bo[cur_idx] = NULL;
+ }
+
+ /*
+* Ask host to release resources (+fill release ring),
+* then wait for the release actually happening.
+*/
+ qxl_io_notify_oom(qdev);
+ for (try = 0; try < 20 && atomic_read(>release_count) > 0; try++)
+ msleep(20);
+
qxl_gem_fini(qdev);
qxl_bo_fini(qdev);
flush_work(>gc_work);
diff --git a/drivers/gpu/drm/qxl/qxl_release.c
b/drivers/gpu/drm/qxl/qxl_release.c
index 28013fd1f8ea..43a5436853b7 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -196,6 +196,7 @@ qxl_release_free(struct qxl_device *qdev,
qxl_release_free_list(release);
kfree(release);
}
+ atomic_dec(>release_count);
}
static int qxl_release_bo_alloc(struct qxl_device *qdev,
@@ -344,6 +345,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev,
unsigned long size,
*rbo = NULL;
return idr_ret;
}
+ atomic_inc(>release_count);
mutex_lock(>release_mutex);
if (qdev->current_release_bo_offset[cur_idx] + 1 >=
releases_per_bo[cur_idx]) {
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v4 4/5] drm/qxl: handle shadow in primary destroy
qxl_primary_atomic_disable must check whenever the framebuffer bo has a
shadow surface and in case it has check the shadow primary status.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_display.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index 60331e31861a..f5ee8cd72b5b 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -562,6 +562,8 @@ static void qxl_primary_atomic_disable(struct drm_plane
*plane,
if (old_state->fb) {
struct qxl_bo *bo = gem_to_qxl_bo(old_state->fb->obj[0]);
+ if (bo->shadow)
+ bo = bo->shadow;
if (bo->is_primary) {
qxl_io_destroy_primary(qdev);
bo->is_primary = false;
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v4 3/5] drm/qxl: release shadow on shutdown
In case we have a shadow surface on shutdown release
it so it doesn't leak.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_display.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index 38d6b596094d..60331e31861a 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -1229,5 +1229,9 @@ int qxl_modeset_init(struct qxl_device *qdev)
void qxl_modeset_fini(struct qxl_device *qdev)
{
+ if (qdev->dumb_shadow_bo) {
+ drm_gem_object_put(>dumb_shadow_bo->tbo.base);
+ qdev->dumb_shadow_bo = NULL;
+ }
qxl_destroy_monitors_object(qdev);
}
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v3 2/4] drm/qxl: unpin release objects
> > Just calling ttm_bo_unpin() here makes lockdep unhappy.
>
> How does that one splat? But yeah if that's a problem should be
> explained in the comment. I'd then also only do a pin_count--; to make
> sure you can still catch other pin leaks if you have them. Setting it
> to 0 kinda defeats the warning.
Figured the unpin is at the completely wrong place while trying to
reproduce the lockdep splat ...
take care,
Gerd
>From 43befab4a935114e8620af62781666fa81288255 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann
Date: Mon, 25 Jan 2021 13:10:50 +0100
Subject: [PATCH] drm/qxl: unpin release objects
Balances the qxl_create_bo(..., pinned=true, ...);
call in qxl_release_bo_alloc().
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_release.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/qxl/qxl_release.c
b/drivers/gpu/drm/qxl/qxl_release.c
index c52412724c26..28013fd1f8ea 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -347,6 +347,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev,
unsigned long size,
mutex_lock(>release_mutex);
if (qdev->current_release_bo_offset[cur_idx] + 1 >=
releases_per_bo[cur_idx]) {
+ qxl_bo_unpin(qdev->current_release_bo[cur_idx]);
qxl_bo_unref(>current_release_bo[cur_idx]);
qdev->current_release_bo_offset[cur_idx] = 0;
qdev->current_release_bo[cur_idx] = NULL;
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v3 2/4] drm/qxl: unpin release objects
On Fri, Jan 22, 2021 at 09:13:42AM +0100, Thomas Zimmermann wrote: > Hi > > Am 20.01.21 um 12:12 schrieb Gerd Hoffmann: > > Balances the qxl_create_bo(..., pinned=true, ...); > > call in qxl_release_bo_alloc(). > > > > Signed-off-by: Gerd Hoffmann > > --- > > drivers/gpu/drm/qxl/qxl_release.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/gpu/drm/qxl/qxl_release.c > > b/drivers/gpu/drm/qxl/qxl_release.c > > index 0fcfc952d5e9..add979cba11b 100644 > > --- a/drivers/gpu/drm/qxl/qxl_release.c > > +++ b/drivers/gpu/drm/qxl/qxl_release.c > > @@ -166,6 +166,7 @@ qxl_release_free_list(struct qxl_release *release) > > entry = container_of(release->bos.next, > > struct qxl_bo_list, tv.head); > > bo = to_qxl_bo(entry->tv.bo); > > + bo->tbo.pin_count = 0; /* ttm_bo_unpin(>tbo); */ > > This code looks like a workaround or a bug. > > AFAICT the only place with pre-pinned BO is qdev->dumb_shadow_bo. Can you > remove the pinned flag entirely and handle pinning as part of > dumb_shadow_bo's code. No, the release objects are pinned too, and they must be pinned (qxl commands are in there, and references are placed in the qxl rings, so allowing them to roam is a non-starter). > if (pin_count) > ttm_bo_unpin(); > WARN_ON(pin_count); /* should always be 0 now */ Well, the pin_count is 1 at this point. No need for the if(). Just calling ttm_bo_unpin() here makes lockdep unhappy. Not calling ttm_bo_unpin() makes ttm_bo_release() throw a WARN() because of the pin. Clearing pin_count (which is how ttm fixes things up in the error path) works. I'm open to better ideas. take care, Gerd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v3 1/4] drm/qxl: use drmm_mode_config_init
Signed-off-by: Gerd Hoffmann
Reviewed-by: Daniel Vetter
---
drivers/gpu/drm/qxl/qxl_display.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index 012bce0cdb65..38d6b596094d 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -1195,7 +1195,9 @@ int qxl_modeset_init(struct qxl_device *qdev)
int i;
int ret;
- drm_mode_config_init(>ddev);
+ ret = drmm_mode_config_init(>ddev);
+ if (ret)
+ return ret;
ret = qxl_create_monitors_object(qdev);
if (ret)
@@ -1228,5 +1230,4 @@ int qxl_modeset_init(struct qxl_device *qdev)
void qxl_modeset_fini(struct qxl_device *qdev)
{
qxl_destroy_monitors_object(qdev);
- drm_mode_config_cleanup(>ddev);
}
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v3 3/4] drm/qxl: release shadow on shutdown
In case we have a shadow surface on shutdown release
it so it doesn't leak.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_display.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index 38d6b596094d..60331e31861a 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -1229,5 +1229,9 @@ int qxl_modeset_init(struct qxl_device *qdev)
void qxl_modeset_fini(struct qxl_device *qdev)
{
+ if (qdev->dumb_shadow_bo) {
+ drm_gem_object_put(>dumb_shadow_bo->tbo.base);
+ qdev->dumb_shadow_bo = NULL;
+ }
qxl_destroy_monitors_object(qdev);
}
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v3 2/4] drm/qxl: unpin release objects
Balances the qxl_create_bo(..., pinned=true, ...); call in qxl_release_bo_alloc(). Signed-off-by: Gerd Hoffmann --- drivers/gpu/drm/qxl/qxl_release.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c index 0fcfc952d5e9..add979cba11b 100644 --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -166,6 +166,7 @@ qxl_release_free_list(struct qxl_release *release) entry = container_of(release->bos.next, struct qxl_bo_list, tv.head); bo = to_qxl_bo(entry->tv.bo); + bo->tbo.pin_count = 0; /* ttm_bo_unpin(>tbo); */ qxl_bo_unref(); list_del(>tv.head); kfree(entry); -- 2.29.2 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v3 4/4] drm/qxl: handle shadow in primary destroy
qxl_primary_atomic_disable must check whenever the framebuffer bo has a
shadow surface and in case it has check the shadow primary status.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_display.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index 60331e31861a..f5ee8cd72b5b 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -562,6 +562,8 @@ static void qxl_primary_atomic_disable(struct drm_plane
*plane,
if (old_state->fb) {
struct qxl_bo *bo = gem_to_qxl_bo(old_state->fb->obj[0]);
+ if (bo->shadow)
+ bo = bo->shadow;
if (bo->is_primary) {
qxl_io_destroy_primary(qdev);
bo->is_primary = false;
--
2.29.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v2 04/20] drm/bochs: Remove references to struct drm_device.pdev
On Tue, Dec 01, 2020 at 11:35:26AM +0100, Thomas Zimmermann wrote:
> Using struct drm_device.pdev is deprecated. Convert bochs to struct
> drm_device.dev. No functional changes.
>
> Signed-off-by: Thomas Zimmermann
> Acked-by: Sam Ravnborg
> Cc: Gerd Hoffmann
Acked-by: Gerd Hoffmann
> ---
> drivers/gpu/drm/bochs/bochs_drv.c | 1 -
> drivers/gpu/drm/bochs/bochs_hw.c | 4 ++--
> 2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/bochs/bochs_drv.c
> b/drivers/gpu/drm/bochs/bochs_drv.c
> index fd454225fd19..b469624fe40d 100644
> --- a/drivers/gpu/drm/bochs/bochs_drv.c
> +++ b/drivers/gpu/drm/bochs/bochs_drv.c
> @@ -121,7 +121,6 @@ static int bochs_pci_probe(struct pci_dev *pdev,
> if (ret)
> goto err_free_dev;
>
> - dev->pdev = pdev;
> pci_set_drvdata(pdev, dev);
>
> ret = bochs_load(dev);
> diff --git a/drivers/gpu/drm/bochs/bochs_hw.c
> b/drivers/gpu/drm/bochs/bochs_hw.c
> index dce4672e3fc8..2d7380a9890e 100644
> --- a/drivers/gpu/drm/bochs/bochs_hw.c
> +++ b/drivers/gpu/drm/bochs/bochs_hw.c
> @@ -110,7 +110,7 @@ int bochs_hw_load_edid(struct bochs_device *bochs)
> int bochs_hw_init(struct drm_device *dev)
> {
> struct bochs_device *bochs = dev->dev_private;
> - struct pci_dev *pdev = dev->pdev;
> + struct pci_dev *pdev = to_pci_dev(dev->dev);
> unsigned long addr, size, mem, ioaddr, iosize;
> u16 id;
>
> @@ -201,7 +201,7 @@ void bochs_hw_fini(struct drm_device *dev)
> release_region(VBE_DISPI_IOPORT_INDEX, 2);
> if (bochs->fb_map)
> iounmap(bochs->fb_map);
> - pci_release_regions(dev->pdev);
> + pci_release_regions(to_pci_dev(dev->dev));
> kfree(bochs->edid);
> }
>
> --
> 2.29.2
>
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v2 05/20] drm/cirrus: Remove references to struct drm_device.pdev
On Tue, Dec 01, 2020 at 11:35:27AM +0100, Thomas Zimmermann wrote: > Using struct drm_device.pdev is deprecated. Convert cirrus to struct > drm_device.dev. No functional changes. > > Signed-off-by: Thomas Zimmermann > Acked-by: Sam Ravnborg > Cc: Gerd Hoffmann Acked-by: Gerd Hoffmann ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v2 14/20] drm/qxl: Remove references to struct drm_device.pdev
On Tue, Dec 01, 2020 at 11:35:36AM +0100, Thomas Zimmermann wrote: > Using struct drm_device.pdev is deprecated. Convert qxl to struct > drm_device.dev. No functional changes. > > Signed-off-by: Thomas Zimmermann > Acked-by: Sam Ravnborg > Cc: Gerd Hoffmann Acked-by: Gerd Hoffmann ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v2 18/20] drm/virtgpu: Remove references to struct drm_device.pdev
On Tue, Dec 01, 2020 at 11:35:40AM +0100, Thomas Zimmermann wrote: > Using struct drm_device.pdev is deprecated. Convert virtgpu to struct > drm_device.dev. No functional changes. > > Signed-off-by: Thomas Zimmermann > Acked-by: Sam Ravnborg > Cc: Gerd Hoffmann Acked-by: Gerd Hoffmann ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] drm/qxl: replace idr_init() by idr_init_base()
On Fri, Nov 06, 2020 at 12:20:16AM +0530, Deepak R Varma wrote:
> idr_init() uses base 0 which is an invalid identifier for this driver.
> The idr_alloc for this driver uses 1 as start value for ID range. The
> new function idr_init_base allows IDR to set the ID lookup from base 1.
> This avoids all lookups that otherwise starts from 0 since 0 is always
> unused / available.
>
> References: commit 6ce711f27500 ("idr: Make 1-based IDRs more efficient")
>
> Signed-off-by: Deepak R Varma
Pushed to drm-misc-next.
thanks,
Gerd
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 3/3] drm/qxl: Remove fbcon acceleration leftovers
On Thu, Oct 29, 2020 at 11:14:28AM +0100, Daniel Vetter wrote:
> These are leftovers from 13aff184ed9f ("drm/qxl: remove dead qxl fbdev
> emulation code").
Acked-by: Gerd Hoffmann
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v2 4/4] drm/qxl: use qxl pin function
Otherwise ttm throws a WARN because we try to pin without a reservation.
Fixes: 9d36d4320462 ("drm/qxl: switch over to the new pin interface")
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_object.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index d3635e3e3267..eb45267d51db 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -145,7 +145,7 @@ int qxl_bo_create(struct qxl_device *qdev,
return r;
}
if (pinned)
- ttm_bo_pin(>tbo);
+ qxl_bo_pin(bo);
*bo_ptr = bo;
return 0;
}
--
2.27.0
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v2 2/4] drm/qxl: release shadow on shutdown
In case we have a shadow surface on shutdown release
it so it doesn't leak.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_display.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index 5bef8f121e54..1d9c51022be4 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -1220,5 +1220,9 @@ int qxl_modeset_init(struct qxl_device *qdev)
void qxl_modeset_fini(struct qxl_device *qdev)
{
+ if (qdev->dumb_shadow_bo) {
+ drm_gem_object_put(>dumb_shadow_bo->tbo.base);
+ qdev->dumb_shadow_bo = NULL;
+ }
qxl_destroy_monitors_object(qdev);
}
--
2.27.0
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v2 1/4] drm/qxl: use drmm_mode_config_init
Signed-off-by: Gerd Hoffmann
Reviewed-by: Daniel Vetter
---
drivers/gpu/drm/qxl/qxl_display.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index 65de1f69af58..5bef8f121e54 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -1186,7 +1186,9 @@ int qxl_modeset_init(struct qxl_device *qdev)
int i;
int ret;
- drm_mode_config_init(>ddev);
+ ret = drmm_mode_config_init(>ddev);
+ if (ret)
+ return ret;
ret = qxl_create_monitors_object(qdev);
if (ret)
@@ -1219,5 +1221,4 @@ int qxl_modeset_init(struct qxl_device *qdev)
void qxl_modeset_fini(struct qxl_device *qdev)
{
qxl_destroy_monitors_object(qdev);
- drm_mode_config_cleanup(>ddev);
}
--
2.27.0
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v2 3/4] drm/qxl: handle shadow in primary destroy
qxl_primary_atomic_disable must check whenever the framebuffer bo has a
shadow surface and in case it has check the shadow primary status.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_display.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index 1d9c51022be4..d133e6c2aaf4 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -561,6 +561,8 @@ static void qxl_primary_atomic_disable(struct drm_plane
*plane,
if (old_state->fb) {
struct qxl_bo *bo = gem_to_qxl_bo(old_state->fb->obj[0]);
+ if (bo->shadow)
+ bo = bo->shadow;
if (bo->is_primary) {
qxl_io_destroy_primary(qdev);
bo->is_primary = false;
--
2.27.0
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH -next] drm/qxl: simplify the return expression of qxl_plane_prepare_fb()
On Mon, Sep 21, 2020 at 09:10:22PM +0800, Qinglang Miao wrote: > Simplify the return expression. Pushed to drm-misc-next. thanks, Gerd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v3 0/6] Add physical display dimensions to spice/virtio-gpu
On Sun, Sep 27, 2020 at 06:57:45PM +0400, marcandre.lur...@redhat.com wrote: > From: Marc-André Lureau > > Hi, > > This series improves the support for HiDPI displays with Spice. > The related spice series have already been merged. > > v3: > - simplify the handling of Spice monitor configuration (Frediano) > v2: > - add a patch to "spice: remove the single monitor config logic" (Gerd) Series queued up now. thanks, Gerd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH 1/3] drm/qxl: use drmm_mode_config_init
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_display.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index fa79688013b7..4be04eaf7f37 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -1190,7 +1190,9 @@ int qxl_modeset_init(struct qxl_device *qdev)
int i;
int ret;
- drm_mode_config_init(>ddev);
+ ret = drmm_mode_config_init(>ddev);
+ if (ret)
+ return ret;
ret = qxl_create_monitors_object(qdev);
if (ret)
@@ -1223,5 +1225,4 @@ int qxl_modeset_init(struct qxl_device *qdev)
void qxl_modeset_fini(struct qxl_device *qdev)
{
qxl_destroy_monitors_object(qdev);
- drm_mode_config_cleanup(>ddev);
}
--
2.27.0
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH 2/3] drm/qxl: release shadow on shutdown
In case we have a shadow surface on shutdown release
it so it doesn't leak.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_display.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index 4be04eaf7f37..85dcb7fe56a9 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -1224,5 +1224,9 @@ int qxl_modeset_init(struct qxl_device *qdev)
void qxl_modeset_fini(struct qxl_device *qdev)
{
+ if (qdev->dumb_shadow_bo) {
+ drm_gem_object_put(>dumb_shadow_bo->tbo.base);
+ qdev->dumb_shadow_bo = NULL;
+ }
qxl_destroy_monitors_object(qdev);
}
--
2.27.0
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH 3/3] drm/qxl: handle shadow in primary destroy
qxl_primary_atomic_disable must check whenever the framebuffer bo has a
shadow surface and in case it has check the shadow primary status.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_display.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index 85dcb7fe56a9..3dcbb359e0f5 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -560,6 +560,8 @@ static void qxl_primary_atomic_disable(struct drm_plane
*plane,
if (old_state->fb) {
struct qxl_bo *bo = gem_to_qxl_bo(old_state->fb->obj[0]);
+ if (bo->shadow)
+ bo = bo->shadow;
if (bo->is_primary) {
qxl_io_destroy_primary(qdev);
bo->is_primary = false;
--
2.27.0
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] drm/qxl: Fix build errors
Hi, > I guess things are never quite so easy :-). It looks like Daniel's > patch is in drm-misc-fixes and Sidong's patch is in drm-misc-next. On > their own they're fine, but once they are merged in drm-tip the build > error shows up. Ah, ok. I've already wondered how that got past my build testing. This explains it. thanks for looking into it, Gerd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] drm/qxl: Replace deprecated function in qxl_display
On Sun, May 24, 2020 at 11:26:23AM +0900, Sidong Yang wrote: > Hi, Dave. > > This is resended e-mail for your advice. > > I'm a newbie interested in linux kernel and qxl module. > Please check this patch and give me advice for me. > Also I'll be glad if there is any task that you bothered. > Thanks. > > Sidong. > > Replace deprecated function drm_modeset_lock/unlock_all with > helper function DRM_MODESET_LOCK_ALL_BEGIN/END. > > Signed-off-by: Sidong Yang Queued for drm-misc-next. thanks, Gerd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v2] drm/qxl: don't take vga ports on rev5+
qemu 5.0 introduces a new qxl hardware revision 5. Unlike revision 4
(and below) the device doesn't switch back into vga compatibility mode
when someone touches the vga ports. So we don't have to reserve the
vga ports any more to avoid that happening.
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/qxl/qxl_drv.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index 13872b882775..6e7f16f4cec7 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -96,7 +96,7 @@ qxl_pci_probe(struct pci_dev *pdev, const struct
pci_device_id *ent)
if (ret)
goto disable_pci;
- if (is_vga(pdev)) {
+ if (is_vga(pdev) && pdev->revision < 5) {
ret = vga_get_interruptible(pdev, VGA_RSRC_LEGACY_IO);
if (ret) {
DRM_ERROR("can't get legacy vga ioports\n");
@@ -127,7 +127,7 @@ qxl_pci_probe(struct pci_dev *pdev, const struct
pci_device_id *ent)
unload:
qxl_device_fini(qdev);
put_vga:
- if (is_vga(pdev))
+ if (is_vga(pdev) && pdev->revision < 5)
vga_put(pdev, VGA_RSRC_LEGACY_IO);
disable_pci:
pci_disable_device(pdev);
@@ -155,7 +155,7 @@ qxl_pci_remove(struct pci_dev *pdev)
drm_dev_unregister(dev);
drm_atomic_helper_shutdown(dev);
- if (is_vga(pdev))
+ if (is_vga(pdev) && pdev->revision < 5)
vga_put(pdev, VGA_RSRC_LEGACY_IO);
}
--
2.18.4
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 1/2] qxl-wddm-dod: fix behavior on v5 device
Hi,
> @@ -4795,7 +4796,8 @@ NTSTATUS QxlDevice::HWClose(void)
> {
> PAGED_CODE();
> QxlClose();
> -if (m_bUefiMode)
> +/* QXL device rev 5+ requires explicit reset to switch to VGA mode */
> +if (m_bUefiMode || m_pQxlDod->Revision() > 4)
> {
> DbgPrint(TRACE_LEVEL_INFORMATION, ("%s: Resetting the device\n",
> __FUNCTION__));
> WRITE_PORT_UCHAR((PUCHAR)(m_IoBase + QXL_IO_RESET), 0);
I'm wondering why this is conditional in the first place?
Isn't it a good idea to reset the device on close no matter what?
take care,
Gerd
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] drm/qxl: qxl_release use after free
On Wed, Apr 29, 2020 at 12:01:24PM +0300, Vasily Averin wrote:
> qxl_release should not be accesses after qxl_push_*_ring_release() calls:
> userspace driver can process submitted command quickly, move qxl_release
> into release_ring, generate interrupt and trigger garbage collector.
>
> It can lead to crashes in qxl driver or trigger memory corruption
> in some kmalloc-192 slab object
>
> Gerd Hoffmann proposes to swap the qxl_release_fence_buffer_objects() +
> qxl_push_{cursor,command}_ring_release() calls to close that race window.
>
> cc: sta...@vger.kernel.org
> Fixes: f64122c1f6ad ("drm: add new QXL driver. (v1.4)")
> Signed-off-by: Vasily Averin
Pushed to drm-misc-fixes.
thanks,
Gerd
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] drm/qxl: lost qxl_bo_kunmap_atomic_page in qxl_image_init_helper()
On Mon, Apr 27, 2020 at 10:55:27AM +0300, Vasily Averin wrote:
> Signed-off-by: Vasily Averin
> ---
> drivers/gpu/drm/qxl/qxl_image.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/qxl/qxl_image.c b/drivers/gpu/drm/qxl/qxl_image.c
> index 43688ecdd8a0..7270da62fc29 100644
> --- a/drivers/gpu/drm/qxl/qxl_image.c
> +++ b/drivers/gpu/drm/qxl/qxl_image.c
> @@ -212,6 +212,7 @@ qxl_image_init_helper(struct qxl_device *qdev,
> break;
> default:
> DRM_ERROR("unsupported image bit depth\n");
> + qxl_bo_kunmap_atomic_page(qdev, image_bo, ptr);
> return -EINVAL; /* TODO: cleanup */
I guess you can ditch the TODO comment now, it's done ;)
take care,
Gerd
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 2/2] drm/qxl: qxl_release leak in qxl_hw_surface_alloc()
On Mon, Apr 27, 2020 at 08:32:51AM +0300, Vasily Averin wrote:
> Cc: sta...@vger.kernel.org
> Fixes: 8002db6336dd ("qxl: convert qxl driver to proper use for reservations")
> Signed-off-by: Vasily Averin
Both patches pushed to drm-misc-fixes.
thanks,
Gerd
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH Resend] drm/qxl: Use correct notify port address when creating cursor ring
On Fri, Apr 24, 2020 at 05:57:37PM +0800, Huacai Chen wrote: > Hi, Gerd > > On Tue, Mar 31, 2020 at 10:53 PM Gerd Hoffmann wrote: > > > > On Tue, Mar 31, 2020 at 02:18:08PM +0800, Huacai Chen wrote: > > > The command ring and cursor ring use different notify port addresses > > > definition: QXL_IO_NOTIFY_CMD and QXL_IO_NOTIFY_CURSOR. However, in > > > qxl_device_init() we use QXL_IO_NOTIFY_CMD to create both command ring > > > and cursor ring. This doesn't cause any problems now, because QEMU's > > > behaviors on QXL_IO_NOTIFY_CMD and QXL_IO_NOTIFY_CURSOR are the same. > > > However, QEMU's behavior may be change in future, so let's fix it. > > > > > > P.S.: In the X.org QXL driver, the notify port address of cursor ring > > > is correct. > > > > > > Cc: > > > Signed-off-by: Huacai Chen > > > > Pushed to drm-misc-next. > It seems that this patch hasn't appear in upstream. Was probably to late for the 5.7 merge window, should land in 5.8 cheers, Gerd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 31/44] drm/qxl: Don't use drm_device->dev_private
On Fri, Apr 03, 2020 at 03:58:15PM +0200, Daniel Vetter wrote: > Upcasting using a container_of macro is more typesafe, faster and > easier for the compiler to optimize. > > Signed-off-by: Daniel Vetter > Cc: Dave Airlie > Cc: Gerd Hoffmann > Cc: virtualizat...@lists.linux-foundation.org > Cc: spice-devel@lists.freedesktop.org Acked-by: Gerd Hoffmann ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 30/44] drm/qxl: Use devm_drm_dev_alloc
On Fri, Apr 03, 2020 at 03:58:14PM +0200, Daniel Vetter wrote: > Also need to remove the drm_dev_put from the remove hook. > > Signed-off-by: Daniel Vetter > Cc: Dave Airlie > Cc: Gerd Hoffmann > Cc: virtualizat...@lists.linux-foundation.org > Cc: spice-devel@lists.freedesktop.org Acked-by: Gerd Hoffmann ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH Resend] drm/qxl: Use correct notify port address when creating cursor ring
On Tue, Mar 31, 2020 at 02:18:08PM +0800, Huacai Chen wrote: > The command ring and cursor ring use different notify port addresses > definition: QXL_IO_NOTIFY_CMD and QXL_IO_NOTIFY_CURSOR. However, in > qxl_device_init() we use QXL_IO_NOTIFY_CMD to create both command ring > and cursor ring. This doesn't cause any problems now, because QEMU's > behaviors on QXL_IO_NOTIFY_CMD and QXL_IO_NOTIFY_CURSOR are the same. > However, QEMU's behavior may be change in future, so let's fix it. > > P.S.: In the X.org QXL driver, the notify port address of cursor ring > is correct. > > Cc: > Signed-off-by: Huacai Chen Pushed to drm-misc-next. thanks, Gerd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v3 1/2] virtio-video: Add virtio video device specification
Hi,
> > With a feature flag both driver and device can choose whenever they want
> > support v1 or v2 or both. With a version config field this is more
> > limited, the device can't decide to support both. So the bonus points
> > for a smooth transition go to the feature flags not the version field ;)
>
> I agree that feature flags would be preferable in general, but I'm
> concerned by the fact that there is (IIUC) a limited number of them.
We have 64 total, some reserved, 24 are available to devices right now,
see https://www.kraxel.org/virtio/virtio-v1.1-cs01-video-v3.html#x1-130002
> Video tends to change significantly over time, and to have optional
> features that would also be presented as feature flags. After a while
> we may run out of them, while a new protocol version would allow us to
> extend the config struct with some new flags. Or am I missing
> something?
Not everything needs a feature flag. For example we have
VIRTIO_VIDEO_CMD_QUERY_CAPABILITY, and we can add new video formats
without needing a feature flag. Maybe it is a good idea to explicitly
say in the specs that this can happen and that the driver should simply
ignore any unknown format returned by the device.
> I also wonder how "support v1 or v2 or both" would work with feature
> flags. In order to make it possible to opt out of v1, I guess we would
> need "v1 supported" flag to begin with?
The driver can ignore any device without v2 feature flag set.
The device can refuse to accept a driver without v2 support (don't allow
setting the FEATURES_OK bit).
A explicit v1 feature flag would allow the guest driver print a more
specific error message ("device doesn't support v1" instead of "feature
negotiation failed"), but that's it.
cheers,
Gerd
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v3 1/2] virtio-video: Add virtio video device specification
Hi,
> Dmitry's virtio-video driver
> https://patchwork.linuxtv.org/patch/61717/.
> Once it becomes fully functional, I'll post a list of possible
> improvements of protocol.
Cool. Actually implementing things can find design problems
in the protocol you didn't notice earlier.
> > > +\begin{description}
> > > +\item[\field{version}] is the protocol version that the device talks.
> > > + The device MUST set this to 0.
> >
> > What is the intended use case for this?
> >
> > Given that virtio has feature flags to negotiate support for optional
> > features and protocol extensions between driver and device, why do you
> > think this is needed?
>
> While feature flags work well when we "extend" the protocol with an
> optional feature, they don't when we want to "drop" or "modify"
> features.
> For example, I guess it'd be useful when we want:
> * to abandon a non-optional command,
> * to change a non-optional struct's layout,or
> * to change the order of commands in which the device expects to be sent.
>
> Though it might be possible to handle these changes by feature flags,
> I suspect the version number allow us to transition protocols more
> smoothly.
Feature flags can be mandatory, both device and driver can fail
initialization when a specific feature is not supported by the other
end. So in case we did screw up things so badly that we have to
effectively start over (which I hope wouldn't be the case) we can add a
VERSION_2 feature flag for a new set of commands with new structs and
new semantics.
With a feature flag both driver and device can choose whenever they want
support v1 or v2 or both. With a version config field this is more
limited, the device can't decide to support both. So the bonus points
for a smooth transition go to the feature flags not the version field ;)
cheers,
Gerd
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v3 2/2] virtio-video: Define a feature for exported objects from different virtio devices
Hi, > +/* > + * Followed by either > + * - struct virtio_video_mem_entry entries[] > + * for VIRTIO_VIDEO_MEM_TYPE_GUEST_PAGES > + * - struct virtio_video_object_entry entries[] > + * for VIRTIO_VIDEO_MEM_TYPE_VIRTIO_OBJECT Wouldn't that be a single virtio_video_object_entry? Or could it be one per plane? cheers, Gerd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v3 1/2] virtio-video: Add virtio video device specification
On Thu, Feb 06, 2020 at 07:20:57PM +0900, Keiichi Watanabe wrote:
> From: Dmitry Sepp
>
> The virtio video encoder device and decoder device provide functionalities to
> encode and decode video stream respectively.
> Though video encoder and decoder are provided as different devices, they use a
> same protocol.
>
> Signed-off-by: Dmitry Sepp
> Signed-off-by: Keiichi Watanabe
Finally found the time for a closer look.
Pretty good overall, some minor nits below ...
> +\begin{description}
> +\item[\field{version}] is the protocol version that the device talks.
> + The device MUST set this to 0.
What is the intended use case for this?
Given that virtio has feature flags to negotiate support for optional
features and protocol extensions between driver and device, why do you
think this is needed?
> +The format description \field{virtio_video_format_desc} is defined as
> +follows:
> +\begin{lstlisting}
> +enum virtio_video_format {
> +/* Raw formats */
> +VIRTIO_VIDEO_FORMAT_RAW_MIN = 1,
> +VIRTIO_VIDEO_FORMAT_ARGB = VIRTIO_VIDEO_FORMAT_RAW_MIN,
> +VIRTIO_VIDEO_FORMAT_BGRA,
> +VIRTIO_VIDEO_FORMAT_NV12, /* 12 Y/CbCr 4:2:0 */
> +VIRTIO_VIDEO_FORMAT_YUV420, /* 12 YUV 4:2:0 */
> +VIRTIO_VIDEO_FORMAT_YVU420, /* 12 YVU 4:2:0 */
> +VIRTIO_VIDEO_FORMAT_RAW_MAX = VIRTIO_VIDEO_FORMAT_YVU420,
I'm wondering what the *_MIN and *_MAX values here (and elsewhere) are
good for? I doubt drivers would actually loop over formats from min to
max, I'd expect they check for specific formats they can handle instead.
If you want define the range for valid raw formats I'd suggest to leave
some room, so new formats can be added without changing MAX values, i.e.
use -- for example -- RAW_MIN = 0x100, RAW_MAX = 0x1ff, CODED_MIN=0x200,
CODED_MAX=0x2ff. Or just drop them ...
> +struct virtio_video_query_control_level {
> +le32 profile; /* One of VIRTIO_VIDEO_PROFILE_* */
^^^ LEVEL ?
cheers,
Gerd
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
