Re: [PATCH v6 0/3] Add sync object UAPI support to VirtIO-GPU driver
by Khronos as with a compute thread + a graphics > thread. > > I'm not saying one threading model is better than any other, perhaps the > native context using the host driver in the guest is so good, it doesn't > matter. I'm just saying these are the types of discussions we can have if we > tried to get one the Mesa MRs merged first ;-) > >> >> Native contexts pass deqp synchronization tests, they use sync objects >> universally for both GL and VK. Games work, piglit/deqp passing. What >> else you're wanting to test? Turnip? > > > Turnip would also fulfill the requirements, since most of the native context > stuff is already wired for freedreno. > >> >> >> The AMDGPU code has been looked and it looks good. It's a draft for now >> because of the missing sync objects UAPI and other virglrender/Qemu >> changes required to get KMS working. > > > Get it out of draft mode then :-). How long would that take? > > Also, there's crosvm which builds on standard Linux, so I wouldn't consider > QEMU patches as a requirement. Just Mesa/virglrenderer part. > >> >> Maybe it will be acceptable to >> merge the Mesa part once kernel will get sync objects supported, will >> need to revisit it. > > > You can think of my commentary as the following suggestions: > > - You can probably get native contexts and deqp-vk 1.2 working with the > current UAPI > - It would be terrific to see inter-context fence sharing working (with the > wait pushed down to the host), that's something the current UAPI can't do > - Work iteratively (i.e, it's fine to merge Mesa/virglrenderer MRs as > "experimental") and in steps, no need to figure everything out at once > > Now these are just suggestions, and while I think they are good, you can > safely ignore them. > > But there's also the DRM requirements, which state "userspace side must be > fully reviewed and tested to the standards of that user-space project.". So > I think to meet the minimum requirements, I think we should at-least have one > of the following (not all, just one) reviewed: > > 1) venus using the new uapi > 2) gfxstream vk using the new uapi > 3) amdgpu nctx out of "draft" mode and using the new uapi. > 4) virtio-intel using new uapi > 5) turnip using your new uapi forgot to mention this earlier, but https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/23533 Dmitry, you can also add, if you haven't already: Tested-by: Rob Clark > Depending on which one you chose, maybe we can get it done within 1-2 weeks? > >> I'm not opening MR for virtio-intel because it has open questions that >> need to be resolved first. >> >> -- >> Best regards, >> Dmitry >> ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v6 0/3] Add sync object UAPI support to VirtIO-GPU driver
On Wed, May 3, 2023 at 10:07 AM Gurchetan Singh wrote: > > > > On Mon, May 1, 2023 at 8:38 AM Dmitry Osipenko > wrote: >> >> On 4/16/23 14:52, Dmitry Osipenko wrote: >> > We have multiple Vulkan context types that are awaiting for the addition >> > of the sync object DRM UAPI support to the VirtIO-GPU kernel driver: >> > >> > 1. Venus context >> > 2. Native contexts (virtio-freedreno, virtio-intel, virtio-amdgpu) >> > >> > Mesa core supports DRM sync object UAPI, providing Vulkan drivers with a >> > generic fencing implementation that we want to utilize. >> > >> > This patch adds initial sync objects support. It creates fundament for a >> > further fencing improvements. Later on we will want to extend the >> > VirtIO-GPU >> > fencing API with passing fence IDs to host for waiting, it will be a new >> > additional VirtIO-GPU IOCTL and more. Today we have several VirtIO-GPU >> > context >> > drivers in works that require VirtIO-GPU to support sync objects UAPI. >> > >> > The patch is heavily inspired by the sync object UAPI implementation of the >> > MSM driver. >> >> Gerd, do you have any objections to merging this series? >> >> We have AMDGPU [1] and Intel [2] native context WIP drivers depending on >> the sync object support. It is the only part missing from kernel today >> that is wanted by the native context drivers. Otherwise, there are few >> other things in Qemu and virglrenderer left to sort out. >> >> [1] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/21658 >> [2] https://gitlab.freedesktop.org/digetx/mesa/-/commits/native-context-iris > > > I'm not saying this change isn't good, just it's probably possible to > implement the native contexts (even up to even VK1.2) without it. But this > patch series may be the most ergonomic way to do it, given how Mesa is > designed. But you probably want one of Mesa MRs reviewed first before > merging (I added a comment on the amdgpu change) and that is a requirement > [a]. > > [a] "The userspace side must be fully reviewed and tested to the standards of > that user space project. For e.g. mesa this means piglit testcases and review > on the mailing list. This is again to ensure that the new interface actually > gets the job done." -- from the requirements > tbh, the syncobj support is all drm core, the only driver specifics is the ioctl parsing. IMHO existing tests and the two existing consumers are sufficient. (Also, considering that additional non-drm dependencies involved.) If this was for the core drm syncobj implementation, and not just driver ioctl parsing and wiring up the core helpers, I would agree with you. BR, -R ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 2/2] drm/virtio: Support sync objects
On Thu, Mar 23, 2023 at 12:05 PM Dmitry Osipenko
wrote:
>
> Add sync object DRM UAPI support to VirtIO-GPU driver. It's required
> for enabling a full-featured Vulkan fencing by Venus and native context
> VirtIO-GPU Mesa drivers.
>
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/gpu/drm/virtio/virtgpu_drv.c| 3 +-
> drivers/gpu/drm/virtio/virtgpu_submit.c | 211
> include/uapi/drm/virtgpu_drm.h | 16 +-
> 3 files changed, 228 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c
> b/drivers/gpu/drm/virtio/virtgpu_drv.c
> index add075681e18..a22155577152 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> @@ -176,7 +176,8 @@ static const struct drm_driver driver = {
> * If KMS is disabled DRIVER_MODESET and DRIVER_ATOMIC are masked
> * out via drm_device::driver_features:
> */
> - .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER |
> DRIVER_ATOMIC,
> + .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER |
> DRIVER_ATOMIC |
> + DRIVER_SYNCOBJ | DRIVER_SYNCOBJ_TIMELINE,
> .open = virtio_gpu_driver_open,
> .postclose = virtio_gpu_driver_postclose,
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_submit.c
> b/drivers/gpu/drm/virtio/virtgpu_submit.c
> index 2ce2459c6bc2..9ea4390948bf 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_submit.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_submit.c
> @@ -14,11 +14,26 @@
> #include
>
> #include
> +#include
> #include
>
> #include "virtgpu_drv.h"
>
> +struct virtio_gpu_submit_post_dep {
> + struct drm_syncobj *syncobj;
> + struct dma_fence_chain *chain;
> + uint64_t point;
> +};
> +
> struct virtio_gpu_submit {
> + struct virtio_gpu_submit_post_dep *post_deps;
> + unsigned int num_out_syncobjs;
> +
> + struct drm_syncobj **in_syncobjs;
> + unsigned int num_in_syncobjs;
> + uint64_t *in_fence_ids;
> + unsigned int num_in_fence_ids;
> +
> struct virtio_gpu_object_array *buflist;
> struct drm_virtgpu_execbuffer *exbuf;
> struct virtio_gpu_fence *out_fence;
> @@ -58,6 +73,186 @@ static int virtio_gpu_dma_fence_wait(struct
> virtio_gpu_submit *submit,
> return 0;
> }
>
> +static void virtio_gpu_free_syncobjs(struct drm_syncobj **syncobjs,
> +uint32_t nr_syncobjs)
> +{
> + uint32_t i = nr_syncobjs;
> +
> + while (syncobjs && i--) {
Checking syncobjs!=NULL here does at least look a bit funny, as the
condition doesn't change in the loop body. It is not incorrect, it
protects you against the cleanup path where submit->in_syncobjs is
NULL. But if (!syncobjs)\nreturn seems a bit more clear
> + if (syncobjs[i])
> + drm_syncobj_put(syncobjs[i]);
> + }
> +
> + kfree(syncobjs);
> +}
> +
> +static int
> +virtio_gpu_parse_deps(struct virtio_gpu_submit *submit)
> +{
> + struct drm_virtgpu_execbuffer *exbuf = submit->exbuf;
> + struct drm_virtgpu_execbuffer_syncobj syncobj_desc;
> + size_t syncobj_stride = exbuf->syncobj_stride;
> + struct drm_syncobj **syncobjs;
> + int ret = 0, i;
> +
> + if (!submit->num_in_syncobjs)
> + return 0;
> +
> + syncobjs = kcalloc(submit->num_in_syncobjs, sizeof(*syncobjs),
> + GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY);
I *think*, assuming I'm reading where this is called correctly (kinda
wish git would show more lines of context by default) that these don't
need to be NOWARN|NORETRY (same for post_deps). I guess you inherited
this from drm/msm, where I appear to have forgotten to update the
syncobj path in commit f0de40a131d9 ("drm/msm: Reorder lock vs submit
alloc"). I don't see anything obvious that would require NORETRY, but
lockdep should be able to tell you otherwise if needed..
BR,
-R
> + if (!syncobjs)
> + return -ENOMEM;
> +
> + for (i = 0; i < submit->num_in_syncobjs; i++) {
> + uint64_t address = exbuf->in_syncobjs + i * syncobj_stride;
> + struct dma_fence *fence;
> +
> + if (copy_from_user(&syncobj_desc,
> + u64_to_user_ptr(address),
> + min(syncobj_stride,
> sizeof(syncobj_desc {
> + ret = -EFAULT;
> + break;
> + }
> +
> + if (syncobj_desc.flags & ~VIRTGPU_EXECBUF_SYNCOBJ_FLAGS) {
> + ret = -EINVAL;
> + break;
> + }
> +
> + ret = drm_syncobj_find_fence(submit->file,
> syncobj_desc.handle,
> +syncobj_desc.point, 0, &fence);
> + if (ret)
> + break;
> +
> + ret = virtio_gpu_dma_fen
Re: [PATCH v3 1/2] drm/virtio: Refactor job submission code path
On Thu, Mar 23, 2023 at 12:05 PM Dmitry Osipenko
wrote:
>
> Move virtio_gpu_execbuffer_ioctl() into separate virtgpu_submit.c file
> and refactor the code along the way to ease addition of new features to
> the ioctl.
>
> Signed-off-by: Dmitry Osipenko
Reviewed-by: Rob Clark
> ---
> drivers/gpu/drm/virtio/Makefile | 2 +-
> drivers/gpu/drm/virtio/virtgpu_drv.h| 4 +
> drivers/gpu/drm/virtio/virtgpu_ioctl.c | 182 --
> drivers/gpu/drm/virtio/virtgpu_submit.c | 302
> 4 files changed, 307 insertions(+), 183 deletions(-)
> create mode 100644 drivers/gpu/drm/virtio/virtgpu_submit.c
>
> diff --git a/drivers/gpu/drm/virtio/Makefile b/drivers/gpu/drm/virtio/Makefile
> index b99fa4a73b68..d2e1788a8227 100644
> --- a/drivers/gpu/drm/virtio/Makefile
> +++ b/drivers/gpu/drm/virtio/Makefile
> @@ -6,6 +6,6 @@
> virtio-gpu-y := virtgpu_drv.o virtgpu_kms.o virtgpu_gem.o virtgpu_vram.o \
> virtgpu_display.o virtgpu_vq.o \
> virtgpu_fence.o virtgpu_object.o virtgpu_debugfs.o virtgpu_plane.o \
> - virtgpu_ioctl.o virtgpu_prime.o virtgpu_trace_points.o
> + virtgpu_ioctl.o virtgpu_prime.o virtgpu_trace_points.o
> virtgpu_submit.o
>
> obj-$(CONFIG_DRM_VIRTIO_GPU) += virtio-gpu.o
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
> b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index af6ffb696086..4126c384286b 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -486,4 +486,8 @@ void virtio_gpu_vram_unmap_dma_buf(struct device *dev,
>struct sg_table *sgt,
>enum dma_data_direction dir);
>
> +/* virtgpu_submit.c */
> +int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file);
> +
> #endif
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> index da45215a933d..b24b11f25197 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> @@ -38,36 +38,6 @@
> VIRTGPU_BLOB_FLAG_USE_SHAREABLE | \
> VIRTGPU_BLOB_FLAG_USE_CROSS_DEVICE)
>
> -static int virtio_gpu_fence_event_create(struct drm_device *dev,
> -struct drm_file *file,
> -struct virtio_gpu_fence *fence,
> -uint32_t ring_idx)
> -{
> - struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
> - struct virtio_gpu_fence_event *e = NULL;
> - int ret;
> -
> - if (!(vfpriv->ring_idx_mask & BIT_ULL(ring_idx)))
> - return 0;
> -
> - e = kzalloc(sizeof(*e), GFP_KERNEL);
> - if (!e)
> - return -ENOMEM;
> -
> - e->event.type = VIRTGPU_EVENT_FENCE_SIGNALED;
> - e->event.length = sizeof(e->event);
> -
> - ret = drm_event_reserve_init(dev, file, &e->base, &e->event);
> - if (ret)
> - goto free;
> -
> - fence->e = e;
> - return 0;
> -free:
> - kfree(e);
> - return ret;
> -}
> -
> /* Must be called with &virtio_gpu_fpriv.struct_mutex held. */
> static void virtio_gpu_create_context_locked(struct virtio_gpu_device *vgdev,
> struct virtio_gpu_fpriv *vfpriv)
> @@ -108,158 +78,6 @@ static int virtio_gpu_map_ioctl(struct drm_device *dev,
> void *data,
> &virtio_gpu_map->offset);
> }
>
> -/*
> - * Usage of execbuffer:
> - * Relocations need to take into account the full VIRTIO_GPUDrawable size.
> - * However, the command as passed from user space must *not* contain the
> initial
> - * VIRTIO_GPUReleaseInfo struct (first XXX bytes)
> - */
> -static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
> -struct drm_file *file)
> -{
> - struct drm_virtgpu_execbuffer *exbuf = data;
> - struct virtio_gpu_device *vgdev = dev->dev_private;
> - struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
> - struct virtio_gpu_fence *out_fence;
> - int ret;
> - uint32_t *bo_handles = NULL;
> - void __user *user_bo_handles = NULL;
> - struct virtio_gpu_object_array *buflist = NULL;
> - struct sync_file *sync_file;
> - int out_fence_fd = -1;
> - void *buf;
> - uint64_t fence_ctx;
> - uint32_t ring_idx;
> -
> - fence_ctx = vgdev->fence_drv.co
Re: [PATCH v2 1/2] drm/virtio: Refactor job submission code path
On Sun, Mar 19, 2023 at 9:11 AM Dmitry Osipenko
wrote:
>
> Move virtio_gpu_execbuffer_ioctl() into separate virtgpu_submit.c file
> and refactor the code along the way to ease addition of new features to
> the ioctl.
>
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/gpu/drm/virtio/Makefile | 2 +-
> drivers/gpu/drm/virtio/virtgpu_drv.h| 4 +
> drivers/gpu/drm/virtio/virtgpu_ioctl.c | 182 ---
> drivers/gpu/drm/virtio/virtgpu_submit.c | 298
> 4 files changed, 303 insertions(+), 183 deletions(-)
> create mode 100644 drivers/gpu/drm/virtio/virtgpu_submit.c
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_submit.c
> b/drivers/gpu/drm/virtio/virtgpu_submit.c
> new file mode 100644
> index ..a96f9d3285c7
> --- /dev/null
> +++ b/drivers/gpu/drm/virtio/virtgpu_submit.c
> @@ -0,0 +1,298 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright (C) 2015 Red Hat, Inc.
> + * All Rights Reserved.
> + *
> + * Authors:
> + *Dave Airlie
> + *Alon Levy
> + */
> +
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +#include "virtgpu_drv.h"
> +
> +struct virtio_gpu_submit {
> + struct virtio_gpu_object_array *buflist;
> + struct drm_virtgpu_execbuffer *exbuf;
> + struct virtio_gpu_fence *out_fence;
> + struct virtio_gpu_fpriv *vfpriv;
> + struct virtio_gpu_device *vgdev;
> + struct drm_file *file;
> + uint64_t fence_ctx;
> + uint32_t ring_idx;
> + int out_fence_fd;
> + void *buf;
> +};
> +
> +static int virtio_gpu_do_fence_wait(struct virtio_gpu_submit *submit,
> + struct dma_fence *dma_fence)
> +{
> + uint32_t context = submit->fence_ctx + submit->ring_idx;
> +
> + if (dma_fence_match_context(dma_fence, context))
> + return 0;
> +
> + return dma_fence_wait(dma_fence, true);
> +}
> +
> +static int virtio_gpu_dma_fence_wait(struct virtio_gpu_submit *submit,
> +struct dma_fence *fence)
> +{
> + struct dma_fence *itr;
> + int idx, err;
> +
> + dma_fence_array_for_each(itr, idx, fence) {
I guess unwrapping is for the later step of host waits?
At any rate, I think you should use dma_fence_unwrap_for_each() to
handle the fence-chain case as well?
> + err = virtio_gpu_do_fence_wait(submit, itr);
> + if (err)
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static int virtio_gpu_fence_event_create(struct drm_device *dev,
> +struct drm_file *file,
> +struct virtio_gpu_fence *fence,
> +uint32_t ring_idx)
> +{
> + struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
> + struct virtio_gpu_fence_event *e = NULL;
> + int ret;
> +
> + if (!(vfpriv->ring_idx_mask & BIT_ULL(ring_idx)))
> + return 0;
> +
> + e = kzalloc(sizeof(*e), GFP_KERNEL);
> + if (!e)
> + return -ENOMEM;
> +
> + e->event.type = VIRTGPU_EVENT_FENCE_SIGNALED;
> + e->event.length = sizeof(e->event);
> +
> + ret = drm_event_reserve_init(dev, file, &e->base, &e->event);
> + if (ret) {
> + kfree(e);
> + return ret;
> + }
> +
> + fence->e = e;
> +
> + return 0;
> +}
> +
> +static int virtio_gpu_init_submit_buflist(struct virtio_gpu_submit *submit)
> +{
> + struct drm_virtgpu_execbuffer *exbuf = submit->exbuf;
> + uint32_t *bo_handles;
> +
> + if (!exbuf->num_bo_handles)
> + return 0;
> +
> + bo_handles = kvmalloc_array(exbuf->num_bo_handles, sizeof(uint32_t),
> + GFP_KERNEL);
> + if (!bo_handles)
> + return -ENOMEM;
> +
> + if (copy_from_user(bo_handles, u64_to_user_ptr(exbuf->bo_handles),
> + exbuf->num_bo_handles * sizeof(uint32_t))) {
> + kvfree(bo_handles);
> + return -EFAULT;
> + }
> +
> + submit->buflist = virtio_gpu_array_from_handles(submit->file,
> bo_handles,
> +
> exbuf->num_bo_handles);
> + if (!submit->buflist) {
> + kvfree(bo_handles);
> + return -ENOENT;
> + }
> +
> + kvfree(bo_handles);
> +
> + return 0;
> +}
> +
> +static void virtio_gpu_cleanup_submit(struct virtio_gpu_submit *submit)
> +{
> + if (!IS_ERR(submit->buf))
> + kvfree(submit->buf);
> +
> + if (submit->buflist)
> + virtio_gpu_array_put_free(submit->buflist);
> +
> + if (submit->out_fence_fd >= 0)
> + put_unused_fd(submit->out_fence_fd);
> +}
> +
> +static void virtio_gpu_submit(struct virtio_gpu_submit *submit)
> +{
> + virtio_gpu_cmd_submit(submit->vgdev, submit->buf,
[PATCH v6] drm/virtio: Add option to disable KMS support
From: Rob Clark
Add a build option to disable modesetting support. This is useful in
cases where the guest only needs to use the GPU in a headless mode, or
(such as in the CrOS usage) window surfaces are proxied to a host
compositor.
As the modesetting ioctls are a big surface area for potential security
bugs to be found (it's happened in the past, we should assume it will
again in the future), it makes sense to have a build option to disable
those ioctls in cases where they serve no legitimate purpose.
v2: Use more if (IS_ENABLED(...))
v3: Also permit the host to advertise no scanouts
v4: Spiff out commit msg
v5: Make num_scanouts==0 and DRM_VIRTIO_GPU_KMS=n behave the same
v6: Drop conditionally building virtgpu_display.c and early-out of
it's init/fini fxns instead
Signed-off-by: Rob Clark
Reviewed-by: Dmitry Osipenko
Reviewed-by: Javier Martinez Canillas
---
drivers/gpu/drm/virtio/Kconfig | 11 +++
drivers/gpu/drm/virtio/virtgpu_display.c | 6 ++
drivers/gpu/drm/virtio/virtgpu_drv.c | 4
drivers/gpu/drm/virtio/virtgpu_kms.c | 23 ++-
4 files changed, 35 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/virtio/Kconfig b/drivers/gpu/drm/virtio/Kconfig
index 51ec7c3240c9..ea06ff2aa4b4 100644
--- a/drivers/gpu/drm/virtio/Kconfig
+++ b/drivers/gpu/drm/virtio/Kconfig
@@ -11,3 +11,14 @@ config DRM_VIRTIO_GPU
QEMU based VMMs (like KVM or Xen).
If unsure say M.
+
+config DRM_VIRTIO_GPU_KMS
+ bool "Virtio GPU driver modesetting support"
+ depends on DRM_VIRTIO_GPU
+ default y
+ help
+ Enable modesetting support for virtio GPU driver. This can be
+ disabled in cases where only "headless" usage of the GPU is
+ required.
+
+ If unsure, say Y.
diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c
b/drivers/gpu/drm/virtio/virtgpu_display.c
index 9ea7611a9e0f..ad924a8502e9 100644
--- a/drivers/gpu/drm/virtio/virtgpu_display.c
+++ b/drivers/gpu/drm/virtio/virtgpu_display.c
@@ -336,6 +336,9 @@ int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev)
{
int i, ret;
+ if (!vgdev->num_scanouts)
+ return 0;
+
ret = drmm_mode_config_init(vgdev->ddev);
if (ret)
return ret;
@@ -362,6 +365,9 @@ void virtio_gpu_modeset_fini(struct virtio_gpu_device
*vgdev)
{
int i;
+ if (!vgdev->num_scanouts)
+ return;
+
for (i = 0 ; i < vgdev->num_scanouts; ++i)
kfree(vgdev->outputs[i].edid);
}
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c
b/drivers/gpu/drm/virtio/virtgpu_drv.c
index ae97b98750b6..add075681e18 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -172,6 +172,10 @@ MODULE_AUTHOR("Alon Levy");
DEFINE_DRM_GEM_FOPS(virtio_gpu_driver_fops);
static const struct drm_driver driver = {
+ /*
+* If KMS is disabled DRIVER_MODESET and DRIVER_ATOMIC are masked
+* out via drm_device::driver_features:
+*/
.driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER |
DRIVER_ATOMIC,
.open = virtio_gpu_driver_open,
.postclose = virtio_gpu_driver_postclose,
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c
b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 27b7f14dae89..02e5c18c2c75 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -223,12 +223,15 @@ int virtio_gpu_init(struct virtio_device *vdev, struct
drm_device *dev)
num_scanouts, &num_scanouts);
vgdev->num_scanouts = min_t(uint32_t, num_scanouts,
VIRTIO_GPU_MAX_SCANOUTS);
- if (!vgdev->num_scanouts) {
- DRM_ERROR("num_scanouts is zero\n");
- ret = -EINVAL;
- goto err_scanouts;
+
+ if (IS_ENABLED(CONFIG_DRM_VIRTIO_GPU_KMS) || !vgdev->num_scanouts) {
+ DRM_INFO("KMS disabled\n");
+ vgdev->num_scanouts = 0;
+ vgdev->has_edid = false;
+ dev->driver_features &= ~(DRIVER_MODESET | DRIVER_ATOMIC);
+ } else {
+ DRM_INFO("number of scanouts: %d\n", num_scanouts);
}
- DRM_INFO("number of scanouts: %d\n", num_scanouts);
virtio_cread_le(vgdev->vdev, struct virtio_gpu_config,
num_capsets, &num_capsets);
@@ -246,10 +249,12 @@ int virtio_gpu_init(struct virtio_device *vdev, struct
drm_device *dev)
virtio_gpu_get_capsets(vgdev, num_capsets);
if (vgdev->has_edid)
virtio_gpu_cmd_get_edids(vgdev);
- virtio_gpu_cmd_get_display_info(vgdev);
- virtio_gpu_notify(vgdev);
- wait_event_timeout(vgdev->resp_wq, !vgdev->displa
Re: [PATCH v5] drm/virtio: Add option to disable KMS support
On Wed, Mar 1, 2023 at 11:25 PM Gerd Hoffmann wrote:
>
> On Thu, Mar 02, 2023 at 12:39:33AM +0300, Dmitry Osipenko wrote:
> > On 3/1/23 21:54, Rob Clark wrote:
> > > /* virtgpu_display.c */
> > > +#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
> > > int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev);
> > > void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev);
> > > +#else
> > > +static inline int virtio_gpu_modeset_init(struct virtio_gpu_device
> > > *vgdev)
> > > +{
> > > + return 0;
> > > +}
> > > +static inline void virtio_gpu_modeset_fini(struct virtio_gpu_device
> > > *vgdev)
> > > +{
> > > +}
> > > +#endif
> >
> > In v4 Gerd wanted to keep building the virtgpu_display.o and instead add
> > the KSM config check to virtio_gpu_modeset_init/fini().
>
> The main point is that the code workflow should be the same in both
> cases. The patch does that for virtio_gpu_modeset_init() but doesn't
> for virtio_gpu_modeset_fini().
>
> Return early in the functions (and drop the #ifdef here) is how I would
> implement this, but I wouldn't insist on that, there are other ways to
> solve this too ;)
Ahh, true, I guess omitting that one file doesn't save anything and
early return makes for a bit simpler/smaller patch
BR,
-R
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v5] drm/virtio: Add option to disable KMS support
From: Rob Clark
Add a build option to disable modesetting support. This is useful in
cases where the guest only needs to use the GPU in a headless mode, or
(such as in the CrOS usage) window surfaces are proxied to a host
compositor.
As the modesetting ioctls are a big surface area for potential security
bugs to be found (it's happened in the past, we should assume it will
again in the future), it makes sense to have a build option to disable
those ioctls in cases where they serve no legitimate purpose.
v2: Use more if (IS_ENABLED(...))
v3: Also permit the host to advertise no scanouts
v4: Spiff out commit msg
v5: Make num_scanouts==0 and DRM_VIRTIO_GPU_KMS=n behave the same
Signed-off-by: Rob Clark
Reviewed-by: Dmitry Osipenko
Reviewed-by: Javier Martinez Canillas
---
drivers/gpu/drm/virtio/Kconfig | 11 ++
drivers/gpu/drm/virtio/Makefile | 5 -
drivers/gpu/drm/virtio/virtgpu_drv.c | 4
drivers/gpu/drm/virtio/virtgpu_drv.h | 10 +
drivers/gpu/drm/virtio/virtgpu_kms.c | 33 +---
5 files changed, 49 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/virtio/Kconfig b/drivers/gpu/drm/virtio/Kconfig
index 51ec7c3240c9..ea06ff2aa4b4 100644
--- a/drivers/gpu/drm/virtio/Kconfig
+++ b/drivers/gpu/drm/virtio/Kconfig
@@ -11,3 +11,14 @@ config DRM_VIRTIO_GPU
QEMU based VMMs (like KVM or Xen).
If unsure say M.
+
+config DRM_VIRTIO_GPU_KMS
+ bool "Virtio GPU driver modesetting support"
+ depends on DRM_VIRTIO_GPU
+ default y
+ help
+ Enable modesetting support for virtio GPU driver. This can be
+ disabled in cases where only "headless" usage of the GPU is
+ required.
+
+ If unsure, say Y.
diff --git a/drivers/gpu/drm/virtio/Makefile b/drivers/gpu/drm/virtio/Makefile
index b99fa4a73b68..24c7ebe87032 100644
--- a/drivers/gpu/drm/virtio/Makefile
+++ b/drivers/gpu/drm/virtio/Makefile
@@ -4,8 +4,11 @@
# Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
virtio-gpu-y := virtgpu_drv.o virtgpu_kms.o virtgpu_gem.o virtgpu_vram.o \
- virtgpu_display.o virtgpu_vq.o \
+ virtgpu_vq.o \
virtgpu_fence.o virtgpu_object.o virtgpu_debugfs.o virtgpu_plane.o \
virtgpu_ioctl.o virtgpu_prime.o virtgpu_trace_points.o
+virtio-gpu-$(CONFIG_DRM_VIRTIO_GPU_KMS) += \
+ virtgpu_display.o
+
obj-$(CONFIG_DRM_VIRTIO_GPU) += virtio-gpu.o
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c
b/drivers/gpu/drm/virtio/virtgpu_drv.c
index ae97b98750b6..add075681e18 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -172,6 +172,10 @@ MODULE_AUTHOR("Alon Levy");
DEFINE_DRM_GEM_FOPS(virtio_gpu_driver_fops);
static const struct drm_driver driver = {
+ /*
+* If KMS is disabled DRIVER_MODESET and DRIVER_ATOMIC are masked
+* out via drm_device::driver_features:
+*/
.driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER |
DRIVER_ATOMIC,
.open = virtio_gpu_driver_open,
.postclose = virtio_gpu_driver_postclose,
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index af6ffb696086..ffe8faf67247 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -426,8 +426,18 @@ virtio_gpu_cmd_set_scanout_blob(struct virtio_gpu_device
*vgdev,
uint32_t x, uint32_t y);
/* virtgpu_display.c */
+#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev);
void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev);
+#else
+static inline int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev)
+{
+ return 0;
+}
+static inline void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev)
+{
+}
+#endif
/* virtgpu_plane.c */
uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc);
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c
b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 27b7f14dae89..61a1afe2c8c8 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -223,21 +223,26 @@ int virtio_gpu_init(struct virtio_device *vdev, struct
drm_device *dev)
num_scanouts, &num_scanouts);
vgdev->num_scanouts = min_t(uint32_t, num_scanouts,
VIRTIO_GPU_MAX_SCANOUTS);
- if (!vgdev->num_scanouts) {
- DRM_ERROR("num_scanouts is zero\n");
- ret = -EINVAL;
- goto err_scanouts;
+
+ if (IS_ENABLED(CONFIG_DRM_VIRTIO_GPU_KMS) || !vgdev->num_scanouts) {
+ DRM_INFO("KMS disabled\n");
+ vgdev->num_scanouts = 0;
+ vgdev->has_edid = false;
+ dev->driver_features &= ~(DRIVER_MODESET | DRIVER_ATOMIC);
+
[PATCH v4] drm/virtio: Add option to disable KMS support
From: Rob Clark
Add a build option to disable modesetting support. This is useful in
cases where the guest only needs to use the GPU in a headless mode, or
(such as in the CrOS usage) window surfaces are proxied to a host
compositor.
As the modesetting ioctls are a big surface area for potential security
bugs to be found (it's happened in the past, we should assume it will
again in the future), it makes sense to have a build option to disable
those ioctls in cases where they serve no legitimate purpose.
v2: Use more if (IS_ENABLED(...))
v3: Also permit the host to advertise no scanouts
v4: Spiff out commit msg
Signed-off-by: Rob Clark
Reviewed-by: Dmitry Osipenko
---
drivers/gpu/drm/virtio/Kconfig | 11 +++
drivers/gpu/drm/virtio/Makefile | 5 +++-
drivers/gpu/drm/virtio/virtgpu_drv.c | 6 +++-
drivers/gpu/drm/virtio/virtgpu_drv.h | 10 +++
drivers/gpu/drm/virtio/virtgpu_kms.c | 44 ++--
5 files changed, 59 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/virtio/Kconfig b/drivers/gpu/drm/virtio/Kconfig
index 51ec7c3240c9..ea06ff2aa4b4 100644
--- a/drivers/gpu/drm/virtio/Kconfig
+++ b/drivers/gpu/drm/virtio/Kconfig
@@ -11,3 +11,14 @@ config DRM_VIRTIO_GPU
QEMU based VMMs (like KVM or Xen).
If unsure say M.
+
+config DRM_VIRTIO_GPU_KMS
+ bool "Virtio GPU driver modesetting support"
+ depends on DRM_VIRTIO_GPU
+ default y
+ help
+ Enable modesetting support for virtio GPU driver. This can be
+ disabled in cases where only "headless" usage of the GPU is
+ required.
+
+ If unsure, say Y.
diff --git a/drivers/gpu/drm/virtio/Makefile b/drivers/gpu/drm/virtio/Makefile
index b99fa4a73b68..24c7ebe87032 100644
--- a/drivers/gpu/drm/virtio/Makefile
+++ b/drivers/gpu/drm/virtio/Makefile
@@ -4,8 +4,11 @@
# Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
virtio-gpu-y := virtgpu_drv.o virtgpu_kms.o virtgpu_gem.o virtgpu_vram.o \
- virtgpu_display.o virtgpu_vq.o \
+ virtgpu_vq.o \
virtgpu_fence.o virtgpu_object.o virtgpu_debugfs.o virtgpu_plane.o \
virtgpu_ioctl.o virtgpu_prime.o virtgpu_trace_points.o
+virtio-gpu-$(CONFIG_DRM_VIRTIO_GPU_KMS) += \
+ virtgpu_display.o
+
obj-$(CONFIG_DRM_VIRTIO_GPU) += virtio-gpu.o
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c
b/drivers/gpu/drm/virtio/virtgpu_drv.c
index ae97b98750b6..9cb7d6dd3da6 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -172,7 +172,11 @@ MODULE_AUTHOR("Alon Levy");
DEFINE_DRM_GEM_FOPS(virtio_gpu_driver_fops);
static const struct drm_driver driver = {
- .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER |
DRIVER_ATOMIC,
+ .driver_features =
+#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
+ DRIVER_MODESET | DRIVER_ATOMIC |
+#endif
+ DRIVER_GEM | DRIVER_RENDER,
.open = virtio_gpu_driver_open,
.postclose = virtio_gpu_driver_postclose,
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index af6ffb696086..ffe8faf67247 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -426,8 +426,18 @@ virtio_gpu_cmd_set_scanout_blob(struct virtio_gpu_device
*vgdev,
uint32_t x, uint32_t y);
/* virtgpu_display.c */
+#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev);
void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev);
+#else
+static inline int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev)
+{
+ return 0;
+}
+static inline void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev)
+{
+}
+#endif
/* virtgpu_plane.c */
uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc);
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c
b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 27b7f14dae89..1d888e309d6b 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -161,7 +161,8 @@ int virtio_gpu_init(struct virtio_device *vdev, struct
drm_device *dev)
if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_VIRGL))
vgdev->has_virgl_3d = true;
#endif
- if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_EDID)) {
+ if (IS_ENABLED(CONFIG_DRM_VIRTIO_GPU_KMS) &&
+ virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_EDID)) {
vgdev->has_edid = true;
}
if (virtio_has_feature(vgdev->vdev, VIRTIO_RING_F_INDIRECT_DESC)) {
@@ -218,17 +219,28 @@ int virtio_gpu_init(struct virtio_device *vdev, struct
drm_device *dev)
goto err_vbufs;
}
- /* get display info */
- virtio_cread_le(vgdev->vdev, struct virtio_gpu_config,
- num_scanouts, &num_s
Re: [PATCH v3] drm/virtio: Add option to disable KMS support
On Tue, Feb 28, 2023 at 4:34 AM Thomas Zimmermann wrote:
>
> Hi
>
> Am 27.02.23 um 19:15 schrieb Rob Clark:
> > On Mon, Feb 27, 2023 at 9:57 AM Dmitry Osipenko
> > wrote:
> >>
> >> On 2/27/23 20:38, Rob Clark wrote:
> >> ...
> >>> + if (IS_ENABLED(CONFIG_DRM_VIRTIO_GPU_KMS)) {
> >>> + /* get display info */
> >>> + virtio_cread_le(vgdev->vdev, struct virtio_gpu_config,
> >>> + num_scanouts, &num_scanouts);
> >>> + vgdev->num_scanouts = min_t(uint32_t, num_scanouts,
> >>> + VIRTIO_GPU_MAX_SCANOUTS);
> >>> + if (!vgdev->num_scanouts) {
> >>> + /*
> >>> + * Having an EDID but no scanouts is non-sensical,
> >>> + * but it is permitted to have no scanouts and no
> >>> + * EDID (in which case DRIVER_MODESET and
> >>> + * DRIVER_ATOMIC are not advertised)
> >>> + */
> >>> + if (vgdev->has_edid) {
> >>> + DRM_ERROR("num_scanouts is zero\n");
> >>> + ret = -EINVAL;
> >>> + goto err_scanouts;
> >>> + }
> >>> + dev->driver_features &= ~(DRIVER_MODESET |
> >>> DRIVER_ATOMIC);
> >>
> >> If it's now configurable by host, why do we need the
> >> CONFIG_DRM_VIRTIO_GPU_KMS?
> >
> > Because a kernel config option makes it more obvious that
> > modeset/atomic ioctls are blocked. Which makes it more obvious about
> > where any potential security issues apply and where fixes need to get
> > backported to. The config option is the only thing _I_ want,
> > everything else is just a bonus to help other people's use-cases.
>
> I find this very vague. What's the security thread?
The modeset ioctls are a big potential attack surface area. Which in
the case of CrOS VM guests serves no legitimate purpose. (kms is
unused in the guest, instead guest window surfaces are proxied to host
for composition alongside host window surfaces.)
There have been in the past potential security bugs (use-after-free,
etc) found in the kms ioctls. We should assume that there will be
more in the future. So it seems like simple common sense to want to
block unused ioctls.
> And if the config option is useful, shouldn't it be DRM-wide? The
> modesetting ioctl calls are shared among all drivers.
Maybe, if there is a use? The situation of compositing guest windows
in the host seems a bit unique to virtgpu, which is why I went with a
config option specific to virtgpu.
BR,
-R
> Best regards
> Thomas
>
> >
> > BR,
> > -R
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3] drm/virtio: Add option to disable KMS support
On Mon, Feb 27, 2023 at 9:57 AM Dmitry Osipenko
wrote:
>
> On 2/27/23 20:38, Rob Clark wrote:
> ...
> > + if (IS_ENABLED(CONFIG_DRM_VIRTIO_GPU_KMS)) {
> > + /* get display info */
> > + virtio_cread_le(vgdev->vdev, struct virtio_gpu_config,
> > + num_scanouts, &num_scanouts);
> > + vgdev->num_scanouts = min_t(uint32_t, num_scanouts,
> > + VIRTIO_GPU_MAX_SCANOUTS);
> > + if (!vgdev->num_scanouts) {
> > + /*
> > + * Having an EDID but no scanouts is non-sensical,
> > + * but it is permitted to have no scanouts and no
> > + * EDID (in which case DRIVER_MODESET and
> > + * DRIVER_ATOMIC are not advertised)
> > + */
> > + if (vgdev->has_edid) {
> > + DRM_ERROR("num_scanouts is zero\n");
> > + ret = -EINVAL;
> > + goto err_scanouts;
> > + }
> > + dev->driver_features &= ~(DRIVER_MODESET |
> > DRIVER_ATOMIC);
>
> If it's now configurable by host, why do we need the
> CONFIG_DRM_VIRTIO_GPU_KMS?
Because a kernel config option makes it more obvious that
modeset/atomic ioctls are blocked. Which makes it more obvious about
where any potential security issues apply and where fixes need to get
backported to. The config option is the only thing _I_ want,
everything else is just a bonus to help other people's use-cases.
BR,
-R
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3] drm/virtio: Add option to disable KMS support
From: Rob Clark
Add a build option to disable modesetting support. This is useful in
cases where the guest only needs to use the GPU in a headless mode, or
(such as in the CrOS usage) window surfaces are proxied to a host
compositor.
v2: Use more if (IS_ENABLED(...))
v3: Also permit the host to advertise no scanouts
Signed-off-by: Rob Clark
---
drivers/gpu/drm/virtio/Kconfig | 11 +++
drivers/gpu/drm/virtio/Makefile | 5 +++-
drivers/gpu/drm/virtio/virtgpu_drv.c | 6 +++-
drivers/gpu/drm/virtio/virtgpu_drv.h | 10 +++
drivers/gpu/drm/virtio/virtgpu_kms.c | 44 ++--
5 files changed, 59 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/virtio/Kconfig b/drivers/gpu/drm/virtio/Kconfig
index 51ec7c3240c9..ea06ff2aa4b4 100644
--- a/drivers/gpu/drm/virtio/Kconfig
+++ b/drivers/gpu/drm/virtio/Kconfig
@@ -11,3 +11,14 @@ config DRM_VIRTIO_GPU
QEMU based VMMs (like KVM or Xen).
If unsure say M.
+
+config DRM_VIRTIO_GPU_KMS
+ bool "Virtio GPU driver modesetting support"
+ depends on DRM_VIRTIO_GPU
+ default y
+ help
+ Enable modesetting support for virtio GPU driver. This can be
+ disabled in cases where only "headless" usage of the GPU is
+ required.
+
+ If unsure, say Y.
diff --git a/drivers/gpu/drm/virtio/Makefile b/drivers/gpu/drm/virtio/Makefile
index b99fa4a73b68..24c7ebe87032 100644
--- a/drivers/gpu/drm/virtio/Makefile
+++ b/drivers/gpu/drm/virtio/Makefile
@@ -4,8 +4,11 @@
# Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
virtio-gpu-y := virtgpu_drv.o virtgpu_kms.o virtgpu_gem.o virtgpu_vram.o \
- virtgpu_display.o virtgpu_vq.o \
+ virtgpu_vq.o \
virtgpu_fence.o virtgpu_object.o virtgpu_debugfs.o virtgpu_plane.o \
virtgpu_ioctl.o virtgpu_prime.o virtgpu_trace_points.o
+virtio-gpu-$(CONFIG_DRM_VIRTIO_GPU_KMS) += \
+ virtgpu_display.o
+
obj-$(CONFIG_DRM_VIRTIO_GPU) += virtio-gpu.o
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c
b/drivers/gpu/drm/virtio/virtgpu_drv.c
index ae97b98750b6..9cb7d6dd3da6 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -172,7 +172,11 @@ MODULE_AUTHOR("Alon Levy");
DEFINE_DRM_GEM_FOPS(virtio_gpu_driver_fops);
static const struct drm_driver driver = {
- .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER |
DRIVER_ATOMIC,
+ .driver_features =
+#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
+ DRIVER_MODESET | DRIVER_ATOMIC |
+#endif
+ DRIVER_GEM | DRIVER_RENDER,
.open = virtio_gpu_driver_open,
.postclose = virtio_gpu_driver_postclose,
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index af6ffb696086..ffe8faf67247 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -426,8 +426,18 @@ virtio_gpu_cmd_set_scanout_blob(struct virtio_gpu_device
*vgdev,
uint32_t x, uint32_t y);
/* virtgpu_display.c */
+#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev);
void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev);
+#else
+static inline int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev)
+{
+ return 0;
+}
+static inline void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev)
+{
+}
+#endif
/* virtgpu_plane.c */
uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc);
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c
b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 27b7f14dae89..1d888e309d6b 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -161,7 +161,8 @@ int virtio_gpu_init(struct virtio_device *vdev, struct
drm_device *dev)
if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_VIRGL))
vgdev->has_virgl_3d = true;
#endif
- if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_EDID)) {
+ if (IS_ENABLED(CONFIG_DRM_VIRTIO_GPU_KMS) &&
+ virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_EDID)) {
vgdev->has_edid = true;
}
if (virtio_has_feature(vgdev->vdev, VIRTIO_RING_F_INDIRECT_DESC)) {
@@ -218,17 +219,28 @@ int virtio_gpu_init(struct virtio_device *vdev, struct
drm_device *dev)
goto err_vbufs;
}
- /* get display info */
- virtio_cread_le(vgdev->vdev, struct virtio_gpu_config,
- num_scanouts, &num_scanouts);
- vgdev->num_scanouts = min_t(uint32_t, num_scanouts,
- VIRTIO_GPU_MAX_SCANOUTS);
- if (!vgdev->num_scanouts) {
- DRM_ERROR("num_scanouts is zero\n");
- ret = -EINVAL;
- goto err_scanouts;
+ if (IS_ENABLED(CONFIG_DRM_VIRTIO_GPU
Re: [PATCH v2] drm/virtio: Add option to disable KMS support
On Mon, Feb 27, 2023 at 8:16 AM Daniel Vetter wrote:
>
> On Mon, Feb 27, 2023 at 08:01:13AM -0800, Rob Clark wrote:
> > From: Rob Clark
> >
> > Add a build option to disable modesetting support. This is useful in
> > cases where the guest only needs to use the GPU in a headless mode, or
> > (such as in the CrOS usage) window surfaces are proxied to a host
> > compositor.
> >
> > v2: Use more if (IS_ENABLED(...))
> >
> > Signed-off-by: Rob Clark
>
> This feels a bit much like a worksforus solution. Not objecting to landing
> this, but would some kind of feature bit on the virtio hw and
> autodetection in the guest driver side work? Especially if people ever
> want to get this to a Just Works model with standard distros.
I could probably make this also work if the host advertises zero
scanouts. But I don't expect distro's would want to disable this
option, which is why it is "If unsure, say Y". The CrOS guest kernel
already needs a special "virtio-wl" driver, so we are already
venturing outside of "guest is just a generic distro" territory.
BR,
-R
> Usually the argument for compile option is "binary size", but you're
> leaving most of the kms stuff in there so that's clearly not it :-)
> -Daniel
>
>
>
> > ---
> > drivers/gpu/drm/virtio/Kconfig | 11 +
> > drivers/gpu/drm/virtio/Makefile | 5 +++-
> > drivers/gpu/drm/virtio/virtgpu_drv.c | 6 -
> > drivers/gpu/drm/virtio/virtgpu_drv.h | 10
> > drivers/gpu/drm/virtio/virtgpu_kms.c | 35
> > 5 files changed, 50 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/virtio/Kconfig b/drivers/gpu/drm/virtio/Kconfig
> > index 51ec7c3240c9..ea06ff2aa4b4 100644
> > --- a/drivers/gpu/drm/virtio/Kconfig
> > +++ b/drivers/gpu/drm/virtio/Kconfig
> > @@ -11,3 +11,14 @@ config DRM_VIRTIO_GPU
> > QEMU based VMMs (like KVM or Xen).
> >
> > If unsure say M.
> > +
> > +config DRM_VIRTIO_GPU_KMS
> > + bool "Virtio GPU driver modesetting support"
> > + depends on DRM_VIRTIO_GPU
> > + default y
> > + help
> > +Enable modesetting support for virtio GPU driver. This can be
> > +disabled in cases where only "headless" usage of the GPU is
> > +required.
> > +
> > +If unsure, say Y.
> > diff --git a/drivers/gpu/drm/virtio/Makefile
> > b/drivers/gpu/drm/virtio/Makefile
> > index b99fa4a73b68..24c7ebe87032 100644
> > --- a/drivers/gpu/drm/virtio/Makefile
> > +++ b/drivers/gpu/drm/virtio/Makefile
> > @@ -4,8 +4,11 @@
> > # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
> >
> > virtio-gpu-y := virtgpu_drv.o virtgpu_kms.o virtgpu_gem.o virtgpu_vram.o \
> > - virtgpu_display.o virtgpu_vq.o \
> > + virtgpu_vq.o \
> > virtgpu_fence.o virtgpu_object.o virtgpu_debugfs.o virtgpu_plane.o \
> > virtgpu_ioctl.o virtgpu_prime.o virtgpu_trace_points.o
> >
> > +virtio-gpu-$(CONFIG_DRM_VIRTIO_GPU_KMS) += \
> > + virtgpu_display.o
> > +
> > obj-$(CONFIG_DRM_VIRTIO_GPU) += virtio-gpu.o
> > diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c
> > b/drivers/gpu/drm/virtio/virtgpu_drv.c
> > index ae97b98750b6..9cb7d6dd3da6 100644
> > --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> > +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> > @@ -172,7 +172,11 @@ MODULE_AUTHOR("Alon Levy");
> > DEFINE_DRM_GEM_FOPS(virtio_gpu_driver_fops);
> >
> > static const struct drm_driver driver = {
> > - .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER |
> > DRIVER_ATOMIC,
> > + .driver_features =
> > +#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
> > + DRIVER_MODESET | DRIVER_ATOMIC |
> > +#endif
> > + DRIVER_GEM | DRIVER_RENDER,
> > .open = virtio_gpu_driver_open,
> > .postclose = virtio_gpu_driver_postclose,
> >
> > diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
> > b/drivers/gpu/drm/virtio/virtgpu_drv.h
> > index af6ffb696086..ffe8faf67247 100644
> > --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> > +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> > @@ -426,8 +426,18 @@ virtio_gpu_cmd_set_scanout_blob(struct
> > virtio_gpu_device *vgdev,
> > uint32_t x, uint32_t y);
> >
> > /* virtgpu_display.c */
> > +#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
> > int virtio_gpu_modeset_init(struct virtio_gpu_d
[PATCH v2] drm/virtio: Add option to disable KMS support
From: Rob Clark
Add a build option to disable modesetting support. This is useful in
cases where the guest only needs to use the GPU in a headless mode, or
(such as in the CrOS usage) window surfaces are proxied to a host
compositor.
v2: Use more if (IS_ENABLED(...))
Signed-off-by: Rob Clark
---
drivers/gpu/drm/virtio/Kconfig | 11 +
drivers/gpu/drm/virtio/Makefile | 5 +++-
drivers/gpu/drm/virtio/virtgpu_drv.c | 6 -
drivers/gpu/drm/virtio/virtgpu_drv.h | 10
drivers/gpu/drm/virtio/virtgpu_kms.c | 35
5 files changed, 50 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/virtio/Kconfig b/drivers/gpu/drm/virtio/Kconfig
index 51ec7c3240c9..ea06ff2aa4b4 100644
--- a/drivers/gpu/drm/virtio/Kconfig
+++ b/drivers/gpu/drm/virtio/Kconfig
@@ -11,3 +11,14 @@ config DRM_VIRTIO_GPU
QEMU based VMMs (like KVM or Xen).
If unsure say M.
+
+config DRM_VIRTIO_GPU_KMS
+ bool "Virtio GPU driver modesetting support"
+ depends on DRM_VIRTIO_GPU
+ default y
+ help
+ Enable modesetting support for virtio GPU driver. This can be
+ disabled in cases where only "headless" usage of the GPU is
+ required.
+
+ If unsure, say Y.
diff --git a/drivers/gpu/drm/virtio/Makefile b/drivers/gpu/drm/virtio/Makefile
index b99fa4a73b68..24c7ebe87032 100644
--- a/drivers/gpu/drm/virtio/Makefile
+++ b/drivers/gpu/drm/virtio/Makefile
@@ -4,8 +4,11 @@
# Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
virtio-gpu-y := virtgpu_drv.o virtgpu_kms.o virtgpu_gem.o virtgpu_vram.o \
- virtgpu_display.o virtgpu_vq.o \
+ virtgpu_vq.o \
virtgpu_fence.o virtgpu_object.o virtgpu_debugfs.o virtgpu_plane.o \
virtgpu_ioctl.o virtgpu_prime.o virtgpu_trace_points.o
+virtio-gpu-$(CONFIG_DRM_VIRTIO_GPU_KMS) += \
+ virtgpu_display.o
+
obj-$(CONFIG_DRM_VIRTIO_GPU) += virtio-gpu.o
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c
b/drivers/gpu/drm/virtio/virtgpu_drv.c
index ae97b98750b6..9cb7d6dd3da6 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -172,7 +172,11 @@ MODULE_AUTHOR("Alon Levy");
DEFINE_DRM_GEM_FOPS(virtio_gpu_driver_fops);
static const struct drm_driver driver = {
- .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER |
DRIVER_ATOMIC,
+ .driver_features =
+#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
+ DRIVER_MODESET | DRIVER_ATOMIC |
+#endif
+ DRIVER_GEM | DRIVER_RENDER,
.open = virtio_gpu_driver_open,
.postclose = virtio_gpu_driver_postclose,
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index af6ffb696086..ffe8faf67247 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -426,8 +426,18 @@ virtio_gpu_cmd_set_scanout_blob(struct virtio_gpu_device
*vgdev,
uint32_t x, uint32_t y);
/* virtgpu_display.c */
+#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev);
void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev);
+#else
+static inline int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev)
+{
+ return 0;
+}
+static inline void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev)
+{
+}
+#endif
/* virtgpu_plane.c */
uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc);
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c
b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 27b7f14dae89..70d87e653d07 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -161,7 +161,8 @@ int virtio_gpu_init(struct virtio_device *vdev, struct
drm_device *dev)
if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_VIRGL))
vgdev->has_virgl_3d = true;
#endif
- if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_EDID)) {
+ if (IS_ENABLED(CONFIG_DRM_VIRTIO_GPU_KMS) &&
+ virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_EDID)) {
vgdev->has_edid = true;
}
if (virtio_has_feature(vgdev->vdev, VIRTIO_RING_F_INDIRECT_DESC)) {
@@ -218,17 +219,19 @@ int virtio_gpu_init(struct virtio_device *vdev, struct
drm_device *dev)
goto err_vbufs;
}
- /* get display info */
- virtio_cread_le(vgdev->vdev, struct virtio_gpu_config,
- num_scanouts, &num_scanouts);
- vgdev->num_scanouts = min_t(uint32_t, num_scanouts,
- VIRTIO_GPU_MAX_SCANOUTS);
- if (!vgdev->num_scanouts) {
- DRM_ERROR("num_scanouts is zero\n");
- ret = -EINVAL;
- goto err_scanouts;
+ if (IS_ENABLED(CONFIG_DRM_VIRTIO_GPU_KMS)) {
+ /* get displa
Re: [PATCH] drm/virtio: Add option to disable KMS support
On Sun, Feb 26, 2023 at 10:38 PM Gerd Hoffmann wrote: > > On Fri, Feb 24, 2023 at 10:02:24AM -0800, Rob Clark wrote: > > From: Rob Clark > > > > Add a build option to disable modesetting support. This is useful in > > cases where the guest only needs to use the GPU in a headless mode, or > > (such as in the CrOS usage) window surfaces are proxied to a host > > compositor. > > Why make that a compile time option? There is a config option for the > number of scanouts (aka virtual displays) a device has. Just set that > to zero (and fix the driver to not consider that configuration an > error). The goal is to not advertise DRIVER_MODESET (and DRIVER_ATOMIC).. I guess that could be done based on whether there are any scanouts, but it would mean making the drm_driver struct non-const. And I think it is legitimate to allow the guest to make this choice, regardless of what the host decides to expose, since it is about the ioctl surface area that the guest kernel exposes to guest userspace. BR, -R ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] drm/virtio: Add option to disable KMS support
From: Rob Clark
Add a build option to disable modesetting support. This is useful in
cases where the guest only needs to use the GPU in a headless mode, or
(such as in the CrOS usage) window surfaces are proxied to a host
compositor.
Signed-off-by: Rob Clark
---
drivers/gpu/drm/virtio/Kconfig | 11 +++
drivers/gpu/drm/virtio/Makefile | 5 -
drivers/gpu/drm/virtio/virtgpu_drv.c | 6 +-
drivers/gpu/drm/virtio/virtgpu_drv.h | 10 ++
drivers/gpu/drm/virtio/virtgpu_kms.c | 6 ++
5 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/virtio/Kconfig b/drivers/gpu/drm/virtio/Kconfig
index 51ec7c3240c9..ea06ff2aa4b4 100644
--- a/drivers/gpu/drm/virtio/Kconfig
+++ b/drivers/gpu/drm/virtio/Kconfig
@@ -11,3 +11,14 @@ config DRM_VIRTIO_GPU
QEMU based VMMs (like KVM or Xen).
If unsure say M.
+
+config DRM_VIRTIO_GPU_KMS
+ bool "Virtio GPU driver modesetting support"
+ depends on DRM_VIRTIO_GPU
+ default y
+ help
+ Enable modesetting support for virtio GPU driver. This can be
+ disabled in cases where only "headless" usage of the GPU is
+ required.
+
+ If unsure, say Y.
diff --git a/drivers/gpu/drm/virtio/Makefile b/drivers/gpu/drm/virtio/Makefile
index b99fa4a73b68..24c7ebe87032 100644
--- a/drivers/gpu/drm/virtio/Makefile
+++ b/drivers/gpu/drm/virtio/Makefile
@@ -4,8 +4,11 @@
# Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
virtio-gpu-y := virtgpu_drv.o virtgpu_kms.o virtgpu_gem.o virtgpu_vram.o \
- virtgpu_display.o virtgpu_vq.o \
+ virtgpu_vq.o \
virtgpu_fence.o virtgpu_object.o virtgpu_debugfs.o virtgpu_plane.o \
virtgpu_ioctl.o virtgpu_prime.o virtgpu_trace_points.o
+virtio-gpu-$(CONFIG_DRM_VIRTIO_GPU_KMS) += \
+ virtgpu_display.o
+
obj-$(CONFIG_DRM_VIRTIO_GPU) += virtio-gpu.o
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c
b/drivers/gpu/drm/virtio/virtgpu_drv.c
index ae97b98750b6..9cb7d6dd3da6 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -172,7 +172,11 @@ MODULE_AUTHOR("Alon Levy");
DEFINE_DRM_GEM_FOPS(virtio_gpu_driver_fops);
static const struct drm_driver driver = {
- .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER |
DRIVER_ATOMIC,
+ .driver_features =
+#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
+ DRIVER_MODESET | DRIVER_ATOMIC |
+#endif
+ DRIVER_GEM | DRIVER_RENDER,
.open = virtio_gpu_driver_open,
.postclose = virtio_gpu_driver_postclose,
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index af6ffb696086..ffe8faf67247 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -426,8 +426,18 @@ virtio_gpu_cmd_set_scanout_blob(struct virtio_gpu_device
*vgdev,
uint32_t x, uint32_t y);
/* virtgpu_display.c */
+#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev);
void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev);
+#else
+static inline int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev)
+{
+ return 0;
+}
+static inline void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev)
+{
+}
+#endif
/* virtgpu_plane.c */
uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc);
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c
b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 27b7f14dae89..293e6f0bf133 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -161,9 +161,11 @@ int virtio_gpu_init(struct virtio_device *vdev, struct
drm_device *dev)
if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_VIRGL))
vgdev->has_virgl_3d = true;
#endif
+#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_EDID)) {
vgdev->has_edid = true;
}
+#endif
if (virtio_has_feature(vgdev->vdev, VIRTIO_RING_F_INDIRECT_DESC)) {
vgdev->has_indirect = true;
}
@@ -218,6 +220,7 @@ int virtio_gpu_init(struct virtio_device *vdev, struct
drm_device *dev)
goto err_vbufs;
}
+#if defined(CONFIG_DRM_VIRTIO_GPU_KMS)
/* get display info */
virtio_cread_le(vgdev->vdev, struct virtio_gpu_config,
num_scanouts, &num_scanouts);
@@ -229,6 +232,7 @@ int virtio_gpu_init(struct virtio_device *vdev, struct
drm_device *dev)
goto err_scanouts;
}
DRM_INFO("number of scanouts: %d\n", num_scanouts);
+#endif
virtio_cread_le(vgdev->vdev, struct virtio_gpu_config,
num_capsets, &num_capsets);
@@ -246,10 +250,12 @@ int virtio_gpu_init(struct virtio_device *vdev, struct
dr
Re: [PATCH] drm/virtio: exbuf->fence_fd unmodified on interrupted wait
On Fri, Jan 27, 2023 at 12:31 AM Ryan Neph wrote:
>
> An interrupted dma_fence_wait() becomes an -ERESTARTSYS returned
> to userspace ioctl(DRM_IOCTL_VIRTGPU_EXECBUFFER) calls, prompting to
> retry the ioctl(), but the passed exbuf->fence_fd has been reset to -1,
> making the retry attempt fail at sync_file_get_fence().
>
> The uapi for DRM_IOCTL_VIRTGPU_EXECBUFFER is changed to retain the
> passed value for exbuf->fence_fd when returning ERESTARTSYS or EINTR.
>
> Fixes: 2cd7b6f08bc4 ("drm/virtio: add in/out fence support for explicit
> synchronization")
> Signed-off-by: Ryan Neph
Reviewed-by: Rob Clark
> ---
>
> drivers/gpu/drm/virtio/virtgpu_ioctl.c | 9 ++---
> include/uapi/drm/virtgpu_drm.h | 3 +++
> 2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> index 9f4a90493aea..ffce4e2a409a 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> @@ -132,6 +132,8 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device
> *dev, void *data,
> uint64_t fence_ctx;
> uint32_t ring_idx;
>
> + exbuf->fence_fd = -1;
> +
> fence_ctx = vgdev->fence_drv.context;
> ring_idx = 0;
>
> @@ -152,8 +154,6 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device
> *dev, void *data,
> ring_idx = exbuf->ring_idx;
> }
>
> - exbuf->fence_fd = -1;
> -
> virtio_gpu_create_context(dev, file);
> if (exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_IN) {
> struct dma_fence *in_fence;
> @@ -173,7 +173,7 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device
> *dev, void *data,
>
> dma_fence_put(in_fence);
> if (ret)
> - return ret;
> + goto out_err;
> }
>
> if (exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_OUT) {
> @@ -259,6 +259,9 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device
> *dev, void *data,
>
> if (out_fence_fd >= 0)
> put_unused_fd(out_fence_fd);
> +out_err:
> + if (ret == -EINTR || ret == -ERESTARTSYS)
> + exbuf->fence_fd = in_fence_fd;
>
> return ret;
> }
> diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
> index 0512fde5e697..ac8d1eed12ab 100644
> --- a/include/uapi/drm/virtgpu_drm.h
> +++ b/include/uapi/drm/virtgpu_drm.h
> @@ -64,6 +64,9 @@ struct drm_virtgpu_map {
> __u32 pad;
> };
>
> +/* For ioctl() returning ERESTARTSYS or EINTR, fence_fd is unmodified.
> + * For all other errors it is set to -1.
> + */
> struct drm_virtgpu_execbuffer {
> __u32 flags;
> __u32 size;
> --
> 2.39.1.456.gfc5497dd1b-goog
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] drm/virtio: Fix GEM handle creation UAF
On Mon, Jan 9, 2023 at 3:28 PM Dmitry Osipenko
wrote:
>
> On 12/17/22 02:33, Rob Clark wrote:
> > From: Rob Clark
> >
> > Userspace can guess the handle value and try to race GEM object creation
> > with handle close, resulting in a use-after-free if we dereference the
> > object after dropping the handle's reference. For that reason, dropping
> > the handle's reference must be done *after* we are done dereferencing
> > the object.
> >
> > Signed-off-by: Rob Clark
> > ---
> > drivers/gpu/drm/virtio/virtgpu_ioctl.c | 19 +--
> > 1 file changed, 17 insertions(+), 2 deletions(-)
>
> Added fixes/stable tags and applied this virtio-gpu patch to misc-fixes.
> The Panfrost patch is untouched.
Thanks.. the panfrost patch was not intended to be part of the same
series (but apparently that is what happens when I send them at the
same time), and was superceded by a patch from Steven Price (commit
4217c6ac8174 ("drm/panfrost: Fix GEM handle creation ref-counting")
already applied to misc-fixes
BR,
-R
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] drm/virtio: Fix GEM handle creation UAF
From: Rob Clark Userspace can guess the handle value and try to race GEM object creation with handle close, resulting in a use-after-free if we dereference the object after dropping the handle's reference. For that reason, dropping the handle's reference must be done *after* we are done dereferencing the object. Signed-off-by: Rob Clark --- drivers/gpu/drm/virtio/virtgpu_ioctl.c | 19 +-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c index a5cb4998..f1c55c1630ca 100644 --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c @@ -366,10 +366,18 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data, drm_gem_object_release(obj); return ret; } - drm_gem_object_put(obj); rc->res_handle = qobj->hw_res_handle; /* similiar to a VM address */ rc->bo_handle = handle; + + /* +* The handle owns the reference now. But we must drop our +* remaining reference *after* we no longer need to dereference +* the obj. Otherwise userspace could guess the handle and +* race closing it from another thread. +*/ + drm_gem_object_put(obj); + return 0; } @@ -731,11 +739,18 @@ static int virtio_gpu_resource_create_blob_ioctl(struct drm_device *dev, drm_gem_object_release(obj); return ret; } - drm_gem_object_put(obj); rc_blob->res_handle = bo->hw_res_handle; rc_blob->bo_handle = handle; + /* +* The handle owns the reference now. But we must drop our +* remaining reference *after* we no longer need to dereference +* the obj. Otherwise userspace could guess the handle and +* race closing it from another thread. +*/ + drm_gem_object_put(obj); + return 0; } -- 2.38.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2] drm/virtio: Spiff out cmd queue/response traces
From: Rob Clark
Add a sequence # for more easily matching up cmd/resp, and the # of free
slots in the virtqueue to more easily see starvation issues.
v2: Fix handling of string fields as well
Signed-off-by: Rob Clark
Reviewed-by: Dmitry Osipenko
---
drivers/gpu/drm/virtio/virtgpu_drv.h | 3 +++
drivers/gpu/drm/virtio/virtgpu_trace.h | 26 +++---
drivers/gpu/drm/virtio/virtgpu_vq.c| 13 ++---
3 files changed, 28 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 9b98470593b0..cdc208d9238c 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -166,6 +166,8 @@ struct virtio_gpu_vbuffer {
struct virtio_gpu_object_array *objs;
struct list_head list;
+
+ uint32_t seqno;
};
struct virtio_gpu_output {
@@ -195,6 +197,7 @@ struct virtio_gpu_queue {
spinlock_t qlock;
wait_queue_head_t ack_queue;
struct work_struct dequeue_work;
+ uint32_t seqno;
};
struct virtio_gpu_drv_capset {
diff --git a/drivers/gpu/drm/virtio/virtgpu_trace.h
b/drivers/gpu/drm/virtio/virtgpu_trace.h
index 711ecc2bd241..031bc77689d5 100644
--- a/drivers/gpu/drm/virtio/virtgpu_trace.h
+++ b/drivers/gpu/drm/virtio/virtgpu_trace.h
@@ -9,40 +9,44 @@
#define TRACE_INCLUDE_FILE virtgpu_trace
DECLARE_EVENT_CLASS(virtio_gpu_cmd,
- TP_PROTO(struct virtqueue *vq, struct virtio_gpu_ctrl_hdr *hdr),
- TP_ARGS(vq, hdr),
+ TP_PROTO(struct virtqueue *vq, struct virtio_gpu_ctrl_hdr *hdr, u32
seqno),
+ TP_ARGS(vq, hdr, seqno),
TP_STRUCT__entry(
__field(int, dev)
__field(unsigned int, vq)
-__field(const char *, name)
+__string(name, vq->name)
__field(u32, type)
__field(u32, flags)
__field(u64, fence_id)
__field(u32, ctx_id)
+__field(u32, num_free)
+__field(u32, seqno)
),
TP_fast_assign(
__entry->dev = vq->vdev->index;
__entry->vq = vq->index;
- __entry->name = vq->name;
+ __assign_str(name, vq->name);
__entry->type = le32_to_cpu(hdr->type);
__entry->flags = le32_to_cpu(hdr->flags);
__entry->fence_id = le64_to_cpu(hdr->fence_id);
__entry->ctx_id = le32_to_cpu(hdr->ctx_id);
+ __entry->num_free = vq->num_free;
+ __entry->seqno = seqno;
),
- TP_printk("vdev=%d vq=%u name=%s type=0x%x flags=0x%x fence_id=%llu
ctx_id=%u",
- __entry->dev, __entry->vq, __entry->name,
+ TP_printk("vdev=%d vq=%u name=%s type=0x%x flags=0x%x fence_id=%llu
ctx_id=%u num_free=%u seqno=%u",
+ __entry->dev, __entry->vq, __get_str(name),
__entry->type, __entry->flags, __entry->fence_id,
- __entry->ctx_id)
+ __entry->ctx_id, __entry->num_free, __entry->seqno)
);
DEFINE_EVENT(virtio_gpu_cmd, virtio_gpu_cmd_queue,
- TP_PROTO(struct virtqueue *vq, struct virtio_gpu_ctrl_hdr *hdr),
- TP_ARGS(vq, hdr)
+ TP_PROTO(struct virtqueue *vq, struct virtio_gpu_ctrl_hdr *hdr, u32
seqno),
+ TP_ARGS(vq, hdr, seqno)
);
DEFINE_EVENT(virtio_gpu_cmd, virtio_gpu_cmd_response,
- TP_PROTO(struct virtqueue *vq, struct virtio_gpu_ctrl_hdr *hdr),
- TP_ARGS(vq, hdr)
+ TP_PROTO(struct virtqueue *vq, struct virtio_gpu_ctrl_hdr *hdr, u32
seqno),
+ TP_ARGS(vq, hdr, seqno)
);
#endif
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c
b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 9ff8660b50ad..a04a9b20896d 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -215,7 +215,7 @@ void virtio_gpu_dequeue_ctrl_func(struct work_struct *work)
list_for_each_entry(entry, &reclaim_list, list) {
resp = (struct virtio_gpu_ctrl_hdr *)entry->resp_buf;
- trace_virtio_gpu_cmd_response(vgdev->ctrlq.vq, resp);
+ trace_virtio_gpu_cmd_response(vgdev->ctrlq.vq, resp,
entry->seqno);
if (resp->type != cpu_to_le32(VIRTIO_GPU_RESP_OK_NODATA)) {
if (le32_to_cpu(resp->type) >=
VIRTIO_GPU_RESP_ERR_UNSPEC) {
@@ -261,6 +261,10 @@ void virtio_gpu_dequeue_cursor_func(struct work_struct
*work)
spin_unlock(&vgdev->cursorq.qlock);
list_for_each_entry_safe(entry, tmp, &reclaim_list, list) {
+ struct virtio_gpu_ctrl_hdr *resp
[PATCH] drm/virtio: Spiff out cmd queue/response traces
From: Rob Clark
Add a sequence # for more easily matching up cmd/resp, and the # of free
slots in the virtqueue to more easily see starvation issues.
Signed-off-by: Rob Clark
---
drivers/gpu/drm/virtio/virtgpu_drv.h | 3 +++
drivers/gpu/drm/virtio/virtgpu_trace.h | 20
drivers/gpu/drm/virtio/virtgpu_vq.c| 13 ++---
3 files changed, 25 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 9b98470593b0..cdc208d9238c 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -166,6 +166,8 @@ struct virtio_gpu_vbuffer {
struct virtio_gpu_object_array *objs;
struct list_head list;
+
+ uint32_t seqno;
};
struct virtio_gpu_output {
@@ -195,6 +197,7 @@ struct virtio_gpu_queue {
spinlock_t qlock;
wait_queue_head_t ack_queue;
struct work_struct dequeue_work;
+ uint32_t seqno;
};
struct virtio_gpu_drv_capset {
diff --git a/drivers/gpu/drm/virtio/virtgpu_trace.h
b/drivers/gpu/drm/virtio/virtgpu_trace.h
index 711ecc2bd241..087e860a66f7 100644
--- a/drivers/gpu/drm/virtio/virtgpu_trace.h
+++ b/drivers/gpu/drm/virtio/virtgpu_trace.h
@@ -9,8 +9,8 @@
#define TRACE_INCLUDE_FILE virtgpu_trace
DECLARE_EVENT_CLASS(virtio_gpu_cmd,
- TP_PROTO(struct virtqueue *vq, struct virtio_gpu_ctrl_hdr *hdr),
- TP_ARGS(vq, hdr),
+ TP_PROTO(struct virtqueue *vq, struct virtio_gpu_ctrl_hdr *hdr, u32
seqno),
+ TP_ARGS(vq, hdr, seqno),
TP_STRUCT__entry(
__field(int, dev)
__field(unsigned int, vq)
@@ -19,6 +19,8 @@ DECLARE_EVENT_CLASS(virtio_gpu_cmd,
__field(u32, flags)
__field(u64, fence_id)
__field(u32, ctx_id)
+__field(u32, num_free)
+__field(u32, seqno)
),
TP_fast_assign(
__entry->dev = vq->vdev->index;
@@ -28,21 +30,23 @@ DECLARE_EVENT_CLASS(virtio_gpu_cmd,
__entry->flags = le32_to_cpu(hdr->flags);
__entry->fence_id = le64_to_cpu(hdr->fence_id);
__entry->ctx_id = le32_to_cpu(hdr->ctx_id);
+ __entry->num_free = vq->num_free;
+ __entry->seqno = seqno;
),
- TP_printk("vdev=%d vq=%u name=%s type=0x%x flags=0x%x fence_id=%llu
ctx_id=%u",
+ TP_printk("vdev=%d vq=%u name=%s type=0x%x flags=0x%x fence_id=%llu
ctx_id=%u num_free=%u seqno=%u",
__entry->dev, __entry->vq, __entry->name,
__entry->type, __entry->flags, __entry->fence_id,
- __entry->ctx_id)
+ __entry->ctx_id, __entry->num_free, __entry->seqno)
);
DEFINE_EVENT(virtio_gpu_cmd, virtio_gpu_cmd_queue,
- TP_PROTO(struct virtqueue *vq, struct virtio_gpu_ctrl_hdr *hdr),
- TP_ARGS(vq, hdr)
+ TP_PROTO(struct virtqueue *vq, struct virtio_gpu_ctrl_hdr *hdr, u32
seqno),
+ TP_ARGS(vq, hdr, seqno)
);
DEFINE_EVENT(virtio_gpu_cmd, virtio_gpu_cmd_response,
- TP_PROTO(struct virtqueue *vq, struct virtio_gpu_ctrl_hdr *hdr),
- TP_ARGS(vq, hdr)
+ TP_PROTO(struct virtqueue *vq, struct virtio_gpu_ctrl_hdr *hdr, u32
seqno),
+ TP_ARGS(vq, hdr, seqno)
);
#endif
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c
b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 9ff8660b50ad..a04a9b20896d 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -215,7 +215,7 @@ void virtio_gpu_dequeue_ctrl_func(struct work_struct *work)
list_for_each_entry(entry, &reclaim_list, list) {
resp = (struct virtio_gpu_ctrl_hdr *)entry->resp_buf;
- trace_virtio_gpu_cmd_response(vgdev->ctrlq.vq, resp);
+ trace_virtio_gpu_cmd_response(vgdev->ctrlq.vq, resp,
entry->seqno);
if (resp->type != cpu_to_le32(VIRTIO_GPU_RESP_OK_NODATA)) {
if (le32_to_cpu(resp->type) >=
VIRTIO_GPU_RESP_ERR_UNSPEC) {
@@ -261,6 +261,10 @@ void virtio_gpu_dequeue_cursor_func(struct work_struct
*work)
spin_unlock(&vgdev->cursorq.qlock);
list_for_each_entry_safe(entry, tmp, &reclaim_list, list) {
+ struct virtio_gpu_ctrl_hdr *resp =
+ (struct virtio_gpu_ctrl_hdr *)entry->resp_buf;
+
+ trace_virtio_gpu_cmd_response(vgdev->cursorq.vq, resp,
entry->seqno);
list_del(&entry->list);
free_vbuf(vgdev, entry);
}
@@ -353,7 +357,8 @@ static int virtio_gpu_queue_ctrl_sgs(struct
virtio_gpu_device *vgdev,
ret = virtqueue_add_sgs(vq, sgs, outcnt, incnt, vbuf,
Re: [PATCH v9 01/11] drm/msm/gem: Prevent blocking within shrinker loop
On Tue, Nov 22, 2022 at 7:00 PM Dmitry Osipenko
wrote:
>
> Consider this scenario:
>
> 1. APP1 continuously creates lots of small GEMs
> 2. APP2 triggers `drop_caches`
> 3. Shrinker starts to evict APP1 GEMs, while APP1 produces new purgeable
>GEMs
> 4. msm_gem_shrinker_scan() returns non-zero number of freed pages
>and causes shrinker to try shrink more
> 5. msm_gem_shrinker_scan() returns non-zero number of freed pages again,
>goto 4
> 6. The APP2 is blocked in `drop_caches` until APP1 stops producing
>purgeable GEMs
>
> To prevent this blocking scenario, check number of remaining pages
> that GPU shrinker couldn't release due to a GEM locking contention
> or shrinking rejection. If there are no remaining pages left to shrink,
> then there is no need to free up more pages and shrinker may break out
> from the loop.
>
> This problem was found during shrinker/madvise IOCTL testing of
> virtio-gpu driver. The MSM driver is affected in the same way.
>
> Signed-off-by: Dmitry Osipenko
Reviewed-by: Rob Clark
> ---
> drivers/gpu/drm/drm_gem.c | 9 +++--
> drivers/gpu/drm/msm/msm_gem_shrinker.c | 8 ++--
> include/drm/drm_gem.h | 4 +++-
> 3 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index b8db675e7fb5..299bca1390aa 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1375,10 +1375,13 @@ EXPORT_SYMBOL(drm_gem_lru_move_tail);
> *
> * @lru: The LRU to scan
> * @nr_to_scan: The number of pages to try to reclaim
> + * @remaining: The number of pages left to reclaim
> * @shrink: Callback to try to shrink/reclaim the object.
> */
> unsigned long
> -drm_gem_lru_scan(struct drm_gem_lru *lru, unsigned nr_to_scan,
> +drm_gem_lru_scan(struct drm_gem_lru *lru,
> +unsigned int nr_to_scan,
> +unsigned long *remaining,
> bool (*shrink)(struct drm_gem_object *obj))
> {
> struct drm_gem_lru still_in_lru;
> @@ -1417,8 +1420,10 @@ drm_gem_lru_scan(struct drm_gem_lru *lru, unsigned
> nr_to_scan,
> * hit shrinker in response to trying to get backing pages
> * for this obj (ie. while it's lock is already held)
> */
> - if (!dma_resv_trylock(obj->resv))
> + if (!dma_resv_trylock(obj->resv)) {
> + *remaining += obj->size >> PAGE_SHIFT;
> goto tail;
> + }
>
> if (shrink(obj)) {
> freed += obj->size >> PAGE_SHIFT;
> diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c
> b/drivers/gpu/drm/msm/msm_gem_shrinker.c
> index 1de14e67f96b..4c8b0ab61ce4 100644
> --- a/drivers/gpu/drm/msm/msm_gem_shrinker.c
> +++ b/drivers/gpu/drm/msm/msm_gem_shrinker.c
> @@ -116,12 +116,14 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct
> shrink_control *sc)
> };
> long nr = sc->nr_to_scan;
> unsigned long freed = 0;
> + unsigned long remaining = 0;
>
> for (unsigned i = 0; (nr > 0) && (i < ARRAY_SIZE(stages)); i++) {
> if (!stages[i].cond)
> continue;
> stages[i].freed =
> - drm_gem_lru_scan(stages[i].lru, nr, stages[i].shrink);
> + drm_gem_lru_scan(stages[i].lru, nr, &remaining,
> +stages[i].shrink);
> nr -= stages[i].freed;
> freed += stages[i].freed;
> }
> @@ -132,7 +134,7 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct
> shrink_control *sc)
> stages[3].freed);
> }
>
> - return (freed > 0) ? freed : SHRINK_STOP;
> + return (freed > 0 && remaining > 0) ? freed : SHRINK_STOP;
> }
>
> #ifdef CONFIG_DEBUG_FS
> @@ -182,10 +184,12 @@ msm_gem_shrinker_vmap(struct notifier_block *nb,
> unsigned long event, void *ptr)
> NULL,
> };
> unsigned idx, unmapped = 0;
> + unsigned long remaining = 0;
>
> for (idx = 0; lrus[idx] && unmapped < vmap_shrink_limit; idx++) {
> unmapped += drm_gem_lru_scan(lrus[idx],
> vmap_shrink_limit - unmapped,
> +&remaining,
> vmap_shrink);
> }
>
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index a17c2f9
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
On Wed, Sep 7, 2022 at 3:25 AM Dmitry Osipenko wrote: > > On 8/23/22 19:47, Rob Clark wrote: > > On Tue, Aug 23, 2022 at 3:01 AM Christian König > > wrote: > >> > >> Am 22.08.22 um 19:26 schrieb Dmitry Osipenko: > >>> On 8/16/22 22:55, Dmitry Osipenko wrote: > >>>> On 8/16/22 15:03, Christian König wrote: > >>>>> Am 16.08.22 um 13:44 schrieb Dmitry Osipenko: > >>>>>> [SNIP] > >>>>>>> The other complication I noticed is that we don't seem to keep around > >>>>>>> the fd after importing to a GEM handle. And I could imagine that > >>>>>>> doing so could cause issues with too many fd's. So I guess the best > >>>>>>> thing is to keep the status quo and let drivers that cannot mmap > >>>>>>> imported buffers just fail mmap? > >>>>>> That actually should be all the drivers excluding those that use > >>>>>> DRM-SHMEM because only DRM-SHMEM uses dma_buf_mmap(), that's why it > >>>>>> works for Panfrost. I'm pretty sure mmaping of imported GEMs doesn't > >>>>>> work for the MSM driver, isn't it? > >>>>>> > >>>>>> Intel and AMD drivers don't allow to map the imported dma-bufs. Both > >>>>>> refuse to do the mapping. > >>>>>> > >>>>>> Although, AMDGPU "succeeds" to do the mapping using > >>>>>> AMDGPU_GEM_DOMAIN_GTT, but then touching the mapping causes bus fault, > >>>>>> hence mapping actually fails. I think it might be the AMDGPU > >>>>>> driver/libdrm bug, haven't checked yet. > >>>>> That's then certainly broken somehow. Amdgpu should nerve ever have > >>>>> allowed to mmap() imported DMA-bufs and the last time I check it didn't. > >>>> I'll take a closer look. So far I can only tell that it's a kernel > >>>> driver issue because once I re-applied this "Don't map imported GEMs" > >>>> patch, AMDGPU began to refuse mapping AMDGPU_GEM_DOMAIN_GTT. > >>>> > >>>>>> So we're back to the point that neither of DRM drivers need to map > >>>>>> imported dma-bufs and this was never tested. In this case this patch is > >>>>>> valid, IMO. > >>>> Actually, I'm now looking at Etnaviv and Nouveau and seems they should > >>>> map imported dma-buf properly. I know that people ran Android on > >>>> Etnaviv. So maybe devices with a separated GPU/display need to map > >>>> imported display BO for Android support. Wish somebody who ran Android > >>>> on one of these devices using upstream drivers could give a definitive > >>>> answer. I may try to test Nouveau later on. > >>>> > >>> Nouveau+Intel combo doesn't work because of [1] that says: > >>> > >>> "Refuse to fault imported pages. This should be handled (if at all) by > >>> redirecting mmap to the exporter." > >>> > >>> [1] > >>> https://elixir.bootlin.com/linux/v5.19/source/drivers/gpu/drm/ttm/ttm_bo_vm.c#L154 > >>> > >>> Interestingly, I noticed that there are IGT tests which check prime > >>> mmaping of Nouveau+Intel [2] (added 9 years ago), but they fail as well, > >>> as expected. The fact that IGT has such tests is interesting because it > >>> suggests that the mapping worked in the past. It's also surprising that > >>> nobody cared to fix the failing tests. For the reference, I checked > >>> v5.18 and today's linux-next. > >>> > >>> [2] > >>> https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/prime_nv_test.c#L132 > >>> > >>> Starting subtest: nv_write_i915_cpu_mmap_read > >>> Received signal SIGBUS. > >>> Stack trace: > >>> #0 [fatal_sig_handler+0x163] > >>> #1 [__sigaction+0x50] > >>> #2 [__igt_uniquereal_main354+0x406] > >>> #3 [main+0x23] > >>> #4 [__libc_start_call_main+0x80] > >>> #5 [__libc_start_main+0x89] > >>> #6 [_start+0x25] > >>> Subtest nv_write_i915_cpu_mmap_read: CRASH (0,005s) > >>> > >>> Starting subtest: nv_write_i915_gtt_mmap_read > >>> Received signal SIGBUS. > >>> Stack trace: > &
Re: [PATCH v1] drm/ttm: Refcount allocated tail pages
On Tue, Sep 6, 2022 at 1:01 PM Daniel Vetter wrote:
>
> On Mon, Aug 15, 2022 at 12:05:19PM +0200, Christian König wrote:
> > Am 15.08.22 um 11:54 schrieb Dmitry Osipenko:
> > > Higher order pages allocated using alloc_pages() aren't refcounted and
> > > they
> > > need to be refcounted, otherwise it's impossible to map them by KVM. This
> > > patch sets the refcount of the tail pages and fixes the KVM memory mapping
> > > faults.
> > >
> > > Without this change guest virgl driver can't map host buffers into guest
> > > and can't provide OpenGL 4.5 profile support to the guest. The host
> > > mappings are also needed for enabling the Venus driver using host GPU
> > > drivers that are utilizing TTM.
> > >
> > > Based on a patch proposed by Trigger Huang.
> >
> > Well I can't count how often I have repeated this: This is an absolutely
> > clear NAK!
> >
> > TTM pages are not reference counted in the first place and because of this
> > giving them to virgl is illegal.
> >
> > Please immediately stop this completely broken approach. We have discussed
> > this multiple times now.
>
> Yeah we need to get this stuff closed for real by tagging them all with
> VM_IO or VM_PFNMAP asap.
>
> It seems ot be a recurring amount of fun that people try to mmap dma-buf
> and then call get_user_pages on them.
>
> Which just doesn't work. I guess this is also why Rob Clark send out that
> dma-buf patch to expos mapping information (i.e. wc vs wb vs uncached).
No, not really.. my patch was simply so that the VMM side of virtgpu
could send the correct cache mode to the guest when handed a dma-buf
;-)
BR,
-R
>
> There seems to be some serious bonghits going on :-/
> -Daniel
>
> >
> > Regards,
> > Christian.
> >
> > >
> > > Cc: sta...@vger.kernel.org
> > > Cc: Trigger Huang
> > > Link:
> > > https://www.collabora.com/news-and-blog/blog/2021/11/26/venus-on-qemu-enabling-new-virtual-vulkan-driver/#qcom1343
> > > Tested-by: Dmitry Osipenko # AMDGPU (Qemu
> > > and crosvm)
> > > Signed-off-by: Dmitry Osipenko
> > > ---
> > > drivers/gpu/drm/ttm/ttm_pool.c | 25 -
> > > 1 file changed, 24 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/ttm/ttm_pool.c
> > > b/drivers/gpu/drm/ttm/ttm_pool.c
> > > index 21b61631f73a..11e92bb149c9 100644
> > > --- a/drivers/gpu/drm/ttm/ttm_pool.c
> > > +++ b/drivers/gpu/drm/ttm/ttm_pool.c
> > > @@ -81,6 +81,7 @@ static struct page *ttm_pool_alloc_page(struct ttm_pool
> > > *pool, gfp_t gfp_flags,
> > > unsigned long attr = DMA_ATTR_FORCE_CONTIGUOUS;
> > > struct ttm_pool_dma *dma;
> > > struct page *p;
> > > + unsigned int i;
> > > void *vaddr;
> > > /* Don't set the __GFP_COMP flag for higher order allocations.
> > > @@ -93,8 +94,10 @@ static struct page *ttm_pool_alloc_page(struct
> > > ttm_pool *pool, gfp_t gfp_flags,
> > > if (!pool->use_dma_alloc) {
> > > p = alloc_pages(gfp_flags, order);
> > > - if (p)
> > > + if (p) {
> > > p->private = order;
> > > + goto ref_tail_pages;
> > > + }
> > > return p;
> > > }
> > > @@ -120,6 +123,23 @@ static struct page *ttm_pool_alloc_page(struct
> > > ttm_pool *pool, gfp_t gfp_flags,
> > > dma->vaddr = (unsigned long)vaddr | order;
> > > p->private = (unsigned long)dma;
> > > +
> > > +ref_tail_pages:
> > > + /*
> > > +* KVM requires mapped tail pages to be refcounted because put_page()
> > > +* is invoked on them in the end of the page fault handling, and thus,
> > > +* tail pages need to be protected from the premature releasing.
> > > +* In fact, KVM page fault handler refuses to map tail pages to guest
> > > +* if they aren't refcounted because hva_to_pfn_remapped() checks the
> > > +* refcount specifically for this case.
> > > +*
> > > +* In particular, unreferenced tail pages result in a KVM "Bad
> > > address"
> > > +* failure for VMMs that use VirtIO-GPU when guest's Mesa VirGL driver
> > > +* accesses mapped host TTM buffer that contains tail pages.
> > > +*/
> > > + for (i = 1; i < 1 << order; i++)
> > > +
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
On Tue, Aug 23, 2022 at 3:01 AM Christian König wrote: > > Am 22.08.22 um 19:26 schrieb Dmitry Osipenko: > > On 8/16/22 22:55, Dmitry Osipenko wrote: > >> On 8/16/22 15:03, Christian König wrote: > >>> Am 16.08.22 um 13:44 schrieb Dmitry Osipenko: > [SNIP] > > The other complication I noticed is that we don't seem to keep around > > the fd after importing to a GEM handle. And I could imagine that > > doing so could cause issues with too many fd's. So I guess the best > > thing is to keep the status quo and let drivers that cannot mmap > > imported buffers just fail mmap? > That actually should be all the drivers excluding those that use > DRM-SHMEM because only DRM-SHMEM uses dma_buf_mmap(), that's why it > works for Panfrost. I'm pretty sure mmaping of imported GEMs doesn't > work for the MSM driver, isn't it? > > Intel and AMD drivers don't allow to map the imported dma-bufs. Both > refuse to do the mapping. > > Although, AMDGPU "succeeds" to do the mapping using > AMDGPU_GEM_DOMAIN_GTT, but then touching the mapping causes bus fault, > hence mapping actually fails. I think it might be the AMDGPU > driver/libdrm bug, haven't checked yet. > >>> That's then certainly broken somehow. Amdgpu should nerve ever have > >>> allowed to mmap() imported DMA-bufs and the last time I check it didn't. > >> I'll take a closer look. So far I can only tell that it's a kernel > >> driver issue because once I re-applied this "Don't map imported GEMs" > >> patch, AMDGPU began to refuse mapping AMDGPU_GEM_DOMAIN_GTT. > >> > So we're back to the point that neither of DRM drivers need to map > imported dma-bufs and this was never tested. In this case this patch is > valid, IMO. > >> Actually, I'm now looking at Etnaviv and Nouveau and seems they should > >> map imported dma-buf properly. I know that people ran Android on > >> Etnaviv. So maybe devices with a separated GPU/display need to map > >> imported display BO for Android support. Wish somebody who ran Android > >> on one of these devices using upstream drivers could give a definitive > >> answer. I may try to test Nouveau later on. > >> > > Nouveau+Intel combo doesn't work because of [1] that says: > > > > "Refuse to fault imported pages. This should be handled (if at all) by > > redirecting mmap to the exporter." > > > > [1] > > https://elixir.bootlin.com/linux/v5.19/source/drivers/gpu/drm/ttm/ttm_bo_vm.c#L154 > > > > Interestingly, I noticed that there are IGT tests which check prime > > mmaping of Nouveau+Intel [2] (added 9 years ago), but they fail as well, > > as expected. The fact that IGT has such tests is interesting because it > > suggests that the mapping worked in the past. It's also surprising that > > nobody cared to fix the failing tests. For the reference, I checked > > v5.18 and today's linux-next. > > > > [2] > > https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/prime_nv_test.c#L132 > > > > Starting subtest: nv_write_i915_cpu_mmap_read > > Received signal SIGBUS. > > Stack trace: > > #0 [fatal_sig_handler+0x163] > > #1 [__sigaction+0x50] > > #2 [__igt_uniquereal_main354+0x406] > > #3 [main+0x23] > > #4 [__libc_start_call_main+0x80] > > #5 [__libc_start_main+0x89] > > #6 [_start+0x25] > > Subtest nv_write_i915_cpu_mmap_read: CRASH (0,005s) > > > > Starting subtest: nv_write_i915_gtt_mmap_read > > Received signal SIGBUS. > > Stack trace: > > #0 [fatal_sig_handler+0x163] > > #1 [__sigaction+0x50] > > #2 [__igt_uniquereal_main354+0x33d] > > #3 [main+0x23] > > #4 [__libc_start_call_main+0x80] > > #5 [__libc_start_main+0x89] > > #6 [_start+0x25] > > Subtest nv_write_i915_gtt_mmap_read: CRASH (0,004s) > > > > I'm curious about the Etnaviv driver because it uses own shmem > > implementation and maybe it has a working mmaping of imported GEMs since > > it imports the dma-buf pages into Entaviv BO. Although, it should be > > risking to map pages using a different caching attributes (WC) from the > > exporter, which is prohibited on ARM ad then one may try to map imported > > udmabuf. > > > > Apparently, the Intel DG TTM driver should be able to map imported > > dma-buf because it sets TTM_TT_FLAG_EXTERNAL_MAPPABLE. > > Even with that flag set it is illegal to map the pages directly by an > importer. > > If that ever worked then the only real solution is to redirect mmap() > calls on importer BOs to dma_buf_mmap(). Yeah, I think this is the best option. Forcing userspace to hang on to the fd just in case someone calls readpix would be pretty harsh. BR, -R > Regards, > Christian. > > > > > Overall, it still questionable to me whether it's worthwhile to allow > > the mmaping of imported GEMs since only Panfrost/Lima can do it out of > > all drivers and h/w that I tested. Feels like drivers that can do the > > mapping have it just because they can and not because they need. > > >
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
On Tue, Aug 16, 2022 at 4:45 AM Dmitry Osipenko wrote: > > On 8/12/22 18:01, Rob Clark wrote: > > On Fri, Aug 12, 2022 at 7:57 AM Rob Clark wrote: > >> > >> On Fri, Aug 12, 2022 at 4:26 AM Dmitry Osipenko > >> wrote: > >>> > >>> On 8/11/22 02:19, Rob Clark wrote: > >>>> On Wed, Aug 10, 2022 at 3:23 PM Dmitry Osipenko > >>>> wrote: > >>>>> > >>>>> On 8/11/22 01:03, Rob Clark wrote: > >>>>>> On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko > >>>>>> wrote: > >>>>>>> > >>>>>>> On 8/10/22 18:08, Rob Clark wrote: > >>>>>>>> On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter > >>>>>>>> wrote: > >>>>>>>>> > >>>>>>>>> On Wed, Jul 06, 2022 at 10:02:07AM +0300, Dmitry Osipenko wrote: > >>>>>>>>>> On 7/6/22 00:48, Rob Clark wrote: > >>>>>>>>>>> On Tue, Jul 5, 2022 at 4:51 AM Christian König > >>>>>>>>>>> wrote: > >>>>>>>>>>>> > >>>>>>>>>>>> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko: > >>>>>>>>>>>>> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers > >>>>>>>>>>>>> don't > >>>>>>>>>>>>> handle imported dma-bufs properly, which results in mapping of > >>>>>>>>>>>>> something > >>>>>>>>>>>>> else than the imported dma-buf. On NVIDIA Tegra we get a hard > >>>>>>>>>>>>> lockup when > >>>>>>>>>>>>> userspace writes to the memory mapping of a dma-buf that was > >>>>>>>>>>>>> imported into > >>>>>>>>>>>>> Tegra's DRM GEM. > >>>>>>>>>>>>> > >>>>>>>>>>>>> Majority of DRM drivers prohibit mapping of the imported GEM > >>>>>>>>>>>>> objects. > >>>>>>>>>>>>> Mapping of imported GEMs require special care from userspace > >>>>>>>>>>>>> since it > >>>>>>>>>>>>> should sync dma-buf because mapping coherency of the exporter > >>>>>>>>>>>>> device may > >>>>>>>>>>>>> not match the DRM device. Let's prohibit the mapping for all > >>>>>>>>>>>>> DRM drivers > >>>>>>>>>>>>> for consistency. > >>>>>>>>>>>>> > >>>>>>>>>>>>> Suggested-by: Thomas Hellström > >>>>>>>>>>>>> > >>>>>>>>>>>>> Signed-off-by: Dmitry Osipenko > >>>>>>>>>>>> > >>>>>>>>>>>> I'm pretty sure that this is the right approach, but it's > >>>>>>>>>>>> certainly more > >>>>>>>>>>>> than possible that somebody abused this already. > >>>>>>>>>>> > >>>>>>>>>>> I suspect that this is abused if you run deqp cts on android.. > >>>>>>>>>>> ie. all > >>>>>>>>>>> winsys buffers are dma-buf imports from gralloc. And then when > >>>>>>>>>>> you > >>>>>>>>>>> hit readpix... > >>>>>>>>>>> > >>>>>>>>>>> You might only hit this in scenarios with separate gpu and > >>>>>>>>>>> display (or > >>>>>>>>>>> dGPU+iGPU) because self-imports are handled differently in > >>>>>>>>>>> drm_gem_prime_import_dev().. and maybe not in cases where you end > >>>>>>>>>>> up > >>>>>>>>>>> with a blit from tiled/compressed to linear.. maybe that narrows > >>>>>>>>>>> the > >>>>>>>>>>> scope enough to just fix it in u
[PATCH] drm/virtio: Fix same-context optimization
From: Rob Clark
When VIRTGPU_EXECBUF_RING_IDX is used, we should be considering the
timeline that the EB if running on rather than the global driver fence
context.
Fixes: 85c83ea915ed ("drm/virtio: implement context init: allocate an array of
fence contexts")
Signed-off-by: Rob Clark
---
drivers/gpu/drm/virtio/virtgpu_ioctl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index fa2f757583f7..9e6cb3c2666e 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -168,7 +168,7 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device
*dev, void *data,
* array contains any fence from a foreign context.
*/
ret = 0;
- if (!dma_fence_match_context(in_fence,
vgdev->fence_drv.context))
+ if (!dma_fence_match_context(in_fence, fence_ctx + ring_idx))
ret = dma_fence_wait(in_fence, true);
dma_fence_put(in_fence);
--
2.36.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
On Fri, Aug 12, 2022 at 7:57 AM Rob Clark wrote: > > On Fri, Aug 12, 2022 at 4:26 AM Dmitry Osipenko > wrote: > > > > On 8/11/22 02:19, Rob Clark wrote: > > > On Wed, Aug 10, 2022 at 3:23 PM Dmitry Osipenko > > > wrote: > > >> > > >> On 8/11/22 01:03, Rob Clark wrote: > > >>> On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko > > >>> wrote: > > >>>> > > >>>> On 8/10/22 18:08, Rob Clark wrote: > > >>>>> On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter wrote: > > >>>>>> > > >>>>>> On Wed, Jul 06, 2022 at 10:02:07AM +0300, Dmitry Osipenko wrote: > > >>>>>>> On 7/6/22 00:48, Rob Clark wrote: > > >>>>>>>> On Tue, Jul 5, 2022 at 4:51 AM Christian König > > >>>>>>>> wrote: > > >>>>>>>>> > > >>>>>>>>> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko: > > >>>>>>>>>> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers > > >>>>>>>>>> don't > > >>>>>>>>>> handle imported dma-bufs properly, which results in mapping of > > >>>>>>>>>> something > > >>>>>>>>>> else than the imported dma-buf. On NVIDIA Tegra we get a hard > > >>>>>>>>>> lockup when > > >>>>>>>>>> userspace writes to the memory mapping of a dma-buf that was > > >>>>>>>>>> imported into > > >>>>>>>>>> Tegra's DRM GEM. > > >>>>>>>>>> > > >>>>>>>>>> Majority of DRM drivers prohibit mapping of the imported GEM > > >>>>>>>>>> objects. > > >>>>>>>>>> Mapping of imported GEMs require special care from userspace > > >>>>>>>>>> since it > > >>>>>>>>>> should sync dma-buf because mapping coherency of the exporter > > >>>>>>>>>> device may > > >>>>>>>>>> not match the DRM device. Let's prohibit the mapping for all DRM > > >>>>>>>>>> drivers > > >>>>>>>>>> for consistency. > > >>>>>>>>>> > > >>>>>>>>>> Suggested-by: Thomas Hellström > > >>>>>>>>>> Signed-off-by: Dmitry Osipenko > > >>>>>>>>> > > >>>>>>>>> I'm pretty sure that this is the right approach, but it's > > >>>>>>>>> certainly more > > >>>>>>>>> than possible that somebody abused this already. > > >>>>>>>> > > >>>>>>>> I suspect that this is abused if you run deqp cts on android.. ie. > > >>>>>>>> all > > >>>>>>>> winsys buffers are dma-buf imports from gralloc. And then when you > > >>>>>>>> hit readpix... > > >>>>>>>> > > >>>>>>>> You might only hit this in scenarios with separate gpu and display > > >>>>>>>> (or > > >>>>>>>> dGPU+iGPU) because self-imports are handled differently in > > >>>>>>>> drm_gem_prime_import_dev().. and maybe not in cases where you end > > >>>>>>>> up > > >>>>>>>> with a blit from tiled/compressed to linear.. maybe that narrows > > >>>>>>>> the > > >>>>>>>> scope enough to just fix it in userspace? > > >>>>>>> > > >>>>>>> Given that that only drivers which use DRM-SHMEM potentially > > >>>>>>> could've > > >>>>>>> map imported dma-bufs (Panfrost, Lima) and they already don't allow > > >>>>>>> to > > >>>>>>> do that, I think we're good. > > >>>>>> > > >>>>>> So can I have an ack from Rob here or are there still questions that > > >>>>>> this > > >>>>>> might go boom? > > >>>>>&
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
On Fri, Aug 12, 2022 at 4:26 AM Dmitry Osipenko wrote: > > On 8/11/22 02:19, Rob Clark wrote: > > On Wed, Aug 10, 2022 at 3:23 PM Dmitry Osipenko > > wrote: > >> > >> On 8/11/22 01:03, Rob Clark wrote: > >>> On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko > >>> wrote: > >>>> > >>>> On 8/10/22 18:08, Rob Clark wrote: > >>>>> On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter wrote: > >>>>>> > >>>>>> On Wed, Jul 06, 2022 at 10:02:07AM +0300, Dmitry Osipenko wrote: > >>>>>>> On 7/6/22 00:48, Rob Clark wrote: > >>>>>>>> On Tue, Jul 5, 2022 at 4:51 AM Christian König > >>>>>>>> wrote: > >>>>>>>>> > >>>>>>>>> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko: > >>>>>>>>>> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers > >>>>>>>>>> don't > >>>>>>>>>> handle imported dma-bufs properly, which results in mapping of > >>>>>>>>>> something > >>>>>>>>>> else than the imported dma-buf. On NVIDIA Tegra we get a hard > >>>>>>>>>> lockup when > >>>>>>>>>> userspace writes to the memory mapping of a dma-buf that was > >>>>>>>>>> imported into > >>>>>>>>>> Tegra's DRM GEM. > >>>>>>>>>> > >>>>>>>>>> Majority of DRM drivers prohibit mapping of the imported GEM > >>>>>>>>>> objects. > >>>>>>>>>> Mapping of imported GEMs require special care from userspace since > >>>>>>>>>> it > >>>>>>>>>> should sync dma-buf because mapping coherency of the exporter > >>>>>>>>>> device may > >>>>>>>>>> not match the DRM device. Let's prohibit the mapping for all DRM > >>>>>>>>>> drivers > >>>>>>>>>> for consistency. > >>>>>>>>>> > >>>>>>>>>> Suggested-by: Thomas Hellström > >>>>>>>>>> Signed-off-by: Dmitry Osipenko > >>>>>>>>> > >>>>>>>>> I'm pretty sure that this is the right approach, but it's certainly > >>>>>>>>> more > >>>>>>>>> than possible that somebody abused this already. > >>>>>>>> > >>>>>>>> I suspect that this is abused if you run deqp cts on android.. ie. > >>>>>>>> all > >>>>>>>> winsys buffers are dma-buf imports from gralloc. And then when you > >>>>>>>> hit readpix... > >>>>>>>> > >>>>>>>> You might only hit this in scenarios with separate gpu and display > >>>>>>>> (or > >>>>>>>> dGPU+iGPU) because self-imports are handled differently in > >>>>>>>> drm_gem_prime_import_dev().. and maybe not in cases where you end up > >>>>>>>> with a blit from tiled/compressed to linear.. maybe that narrows the > >>>>>>>> scope enough to just fix it in userspace? > >>>>>>> > >>>>>>> Given that that only drivers which use DRM-SHMEM potentially could've > >>>>>>> map imported dma-bufs (Panfrost, Lima) and they already don't allow to > >>>>>>> do that, I think we're good. > >>>>>> > >>>>>> So can I have an ack from Rob here or are there still questions that > >>>>>> this > >>>>>> might go boom? > >>>>>> > >>>>>> Dmitry, since you have a bunch of patches merged now I think would > >>>>>> also be > >>>>>> good to get commit rights so you can drive this more yourself. I've > >>>>>> asked > >>>>>> Daniel Stone to help you out with getting that. > >>>>> > >>>>> I *think* we'd be ok with this on msm, mostly just by dumb luck. > >>>>> Because the dma-buf's we import will be self-impo
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
On Wed, Aug 10, 2022 at 3:23 PM Dmitry Osipenko wrote: > > On 8/11/22 01:03, Rob Clark wrote: > > On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko > > wrote: > >> > >> On 8/10/22 18:08, Rob Clark wrote: > >>> On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter wrote: > >>>> > >>>> On Wed, Jul 06, 2022 at 10:02:07AM +0300, Dmitry Osipenko wrote: > >>>>> On 7/6/22 00:48, Rob Clark wrote: > >>>>>> On Tue, Jul 5, 2022 at 4:51 AM Christian König > >>>>>> wrote: > >>>>>>> > >>>>>>> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko: > >>>>>>>> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't > >>>>>>>> handle imported dma-bufs properly, which results in mapping of > >>>>>>>> something > >>>>>>>> else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup > >>>>>>>> when > >>>>>>>> userspace writes to the memory mapping of a dma-buf that was > >>>>>>>> imported into > >>>>>>>> Tegra's DRM GEM. > >>>>>>>> > >>>>>>>> Majority of DRM drivers prohibit mapping of the imported GEM objects. > >>>>>>>> Mapping of imported GEMs require special care from userspace since it > >>>>>>>> should sync dma-buf because mapping coherency of the exporter device > >>>>>>>> may > >>>>>>>> not match the DRM device. Let's prohibit the mapping for all DRM > >>>>>>>> drivers > >>>>>>>> for consistency. > >>>>>>>> > >>>>>>>> Suggested-by: Thomas Hellström > >>>>>>>> Signed-off-by: Dmitry Osipenko > >>>>>>> > >>>>>>> I'm pretty sure that this is the right approach, but it's certainly > >>>>>>> more > >>>>>>> than possible that somebody abused this already. > >>>>>> > >>>>>> I suspect that this is abused if you run deqp cts on android.. ie. all > >>>>>> winsys buffers are dma-buf imports from gralloc. And then when you > >>>>>> hit readpix... > >>>>>> > >>>>>> You might only hit this in scenarios with separate gpu and display (or > >>>>>> dGPU+iGPU) because self-imports are handled differently in > >>>>>> drm_gem_prime_import_dev().. and maybe not in cases where you end up > >>>>>> with a blit from tiled/compressed to linear.. maybe that narrows the > >>>>>> scope enough to just fix it in userspace? > >>>>> > >>>>> Given that that only drivers which use DRM-SHMEM potentially could've > >>>>> map imported dma-bufs (Panfrost, Lima) and they already don't allow to > >>>>> do that, I think we're good. > >>>> > >>>> So can I have an ack from Rob here or are there still questions that this > >>>> might go boom? > >>>> > >>>> Dmitry, since you have a bunch of patches merged now I think would also > >>>> be > >>>> good to get commit rights so you can drive this more yourself. I've asked > >>>> Daniel Stone to help you out with getting that. > >>> > >>> I *think* we'd be ok with this on msm, mostly just by dumb luck. > >>> Because the dma-buf's we import will be self-import. I'm less sure > >>> about panfrost (src/panfrost/lib/pan_bo.c doesn't seem to have a > >>> special path for imported dma-bufs either, and in that case they won't > >>> be self-imports.. but I guess no one has tried to run android cts on > >>> panfrost). > >> > >> The last time I tried to mmap dma-buf imported to Panfrost didn't work > >> because Panfrost didn't implement something needed for that. I'll need > >> to take a look again because can't recall what it was. > >> > >>> What about something less drastic to start, like (apologies for > >>> hand-edited patch): > >>> > >>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > >>> index 86d670c71286..fc9ec42fa0ab 100644 > >
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
On Wed, Aug 10, 2022 at 12:26 PM Dmitry Osipenko
wrote:
>
> On 8/10/22 18:08, Rob Clark wrote:
> > On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter wrote:
> >>
> >> On Wed, Jul 06, 2022 at 10:02:07AM +0300, Dmitry Osipenko wrote:
> >>> On 7/6/22 00:48, Rob Clark wrote:
> >>>> On Tue, Jul 5, 2022 at 4:51 AM Christian König
> >>>> wrote:
> >>>>>
> >>>>> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
> >>>>>> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
> >>>>>> handle imported dma-bufs properly, which results in mapping of
> >>>>>> something
> >>>>>> else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup
> >>>>>> when
> >>>>>> userspace writes to the memory mapping of a dma-buf that was imported
> >>>>>> into
> >>>>>> Tegra's DRM GEM.
> >>>>>>
> >>>>>> Majority of DRM drivers prohibit mapping of the imported GEM objects.
> >>>>>> Mapping of imported GEMs require special care from userspace since it
> >>>>>> should sync dma-buf because mapping coherency of the exporter device
> >>>>>> may
> >>>>>> not match the DRM device. Let's prohibit the mapping for all DRM
> >>>>>> drivers
> >>>>>> for consistency.
> >>>>>>
> >>>>>> Suggested-by: Thomas Hellström
> >>>>>> Signed-off-by: Dmitry Osipenko
> >>>>>
> >>>>> I'm pretty sure that this is the right approach, but it's certainly more
> >>>>> than possible that somebody abused this already.
> >>>>
> >>>> I suspect that this is abused if you run deqp cts on android.. ie. all
> >>>> winsys buffers are dma-buf imports from gralloc. And then when you
> >>>> hit readpix...
> >>>>
> >>>> You might only hit this in scenarios with separate gpu and display (or
> >>>> dGPU+iGPU) because self-imports are handled differently in
> >>>> drm_gem_prime_import_dev().. and maybe not in cases where you end up
> >>>> with a blit from tiled/compressed to linear.. maybe that narrows the
> >>>> scope enough to just fix it in userspace?
> >>>
> >>> Given that that only drivers which use DRM-SHMEM potentially could've
> >>> map imported dma-bufs (Panfrost, Lima) and they already don't allow to
> >>> do that, I think we're good.
> >>
> >> So can I have an ack from Rob here or are there still questions that this
> >> might go boom?
> >>
> >> Dmitry, since you have a bunch of patches merged now I think would also be
> >> good to get commit rights so you can drive this more yourself. I've asked
> >> Daniel Stone to help you out with getting that.
> >
> > I *think* we'd be ok with this on msm, mostly just by dumb luck.
> > Because the dma-buf's we import will be self-import. I'm less sure
> > about panfrost (src/panfrost/lib/pan_bo.c doesn't seem to have a
> > special path for imported dma-bufs either, and in that case they won't
> > be self-imports.. but I guess no one has tried to run android cts on
> > panfrost).
>
> The last time I tried to mmap dma-buf imported to Panfrost didn't work
> because Panfrost didn't implement something needed for that. I'll need
> to take a look again because can't recall what it was.
>
> > What about something less drastic to start, like (apologies for
> > hand-edited patch):
> >
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 86d670c71286..fc9ec42fa0ab 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object
> > *obj, unsigned long obj_size,
> > {
> > int ret;
> >
> > + WARN_ON_ONCE(obj->import_attach);
>
> This will hang NVIDIA Tegra, which is what this patch fixed initially.
> If neither of upstream DRM drivers need to map imported dma-bufs and
> never needed, then why do we need this?
oh, tegra isn't using shmem helpers? I assumed it was. Well my point
was to make a more targeted fail on tegra, and a WARN_ON for everyone
else to make it clear that what they are doing is undefined behavior.
Because so far existing userspace (or well, panfrost and freedreno at
least, those are the two I know or checked) don't make special cases
for mmap'ing against the dmabuf fd against the dmabuf fd instead of
the drm device fd.
I *think* it should work out that we don't hit this path with
freedreno but on android I can't really guarantee or prove it. So
your patch would potentially break existing working userspace. Maybe
it is userspace that isn't portable (but OTOH it isn't like you are
going to be using freedreno on tegra). So why don't you go for a more
targeted fix that only returns an error on hw where this is
problematic?
BR,
-R
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
On Wed, Aug 10, 2022 at 4:47 AM Daniel Vetter wrote:
>
> On Wed, Jul 06, 2022 at 10:02:07AM +0300, Dmitry Osipenko wrote:
> > On 7/6/22 00:48, Rob Clark wrote:
> > > On Tue, Jul 5, 2022 at 4:51 AM Christian König
> > > wrote:
> > >>
> > >> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
> > >>> Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
> > >>> handle imported dma-bufs properly, which results in mapping of something
> > >>> else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup
> > >>> when
> > >>> userspace writes to the memory mapping of a dma-buf that was imported
> > >>> into
> > >>> Tegra's DRM GEM.
> > >>>
> > >>> Majority of DRM drivers prohibit mapping of the imported GEM objects.
> > >>> Mapping of imported GEMs require special care from userspace since it
> > >>> should sync dma-buf because mapping coherency of the exporter device may
> > >>> not match the DRM device. Let's prohibit the mapping for all DRM drivers
> > >>> for consistency.
> > >>>
> > >>> Suggested-by: Thomas Hellström
> > >>> Signed-off-by: Dmitry Osipenko
> > >>
> > >> I'm pretty sure that this is the right approach, but it's certainly more
> > >> than possible that somebody abused this already.
> > >
> > > I suspect that this is abused if you run deqp cts on android.. ie. all
> > > winsys buffers are dma-buf imports from gralloc. And then when you
> > > hit readpix...
> > >
> > > You might only hit this in scenarios with separate gpu and display (or
> > > dGPU+iGPU) because self-imports are handled differently in
> > > drm_gem_prime_import_dev().. and maybe not in cases where you end up
> > > with a blit from tiled/compressed to linear.. maybe that narrows the
> > > scope enough to just fix it in userspace?
> >
> > Given that that only drivers which use DRM-SHMEM potentially could've
> > map imported dma-bufs (Panfrost, Lima) and they already don't allow to
> > do that, I think we're good.
>
> So can I have an ack from Rob here or are there still questions that this
> might go boom?
>
> Dmitry, since you have a bunch of patches merged now I think would also be
> good to get commit rights so you can drive this more yourself. I've asked
> Daniel Stone to help you out with getting that.
I *think* we'd be ok with this on msm, mostly just by dumb luck.
Because the dma-buf's we import will be self-import. I'm less sure
about panfrost (src/panfrost/lib/pan_bo.c doesn't seem to have a
special path for imported dma-bufs either, and in that case they won't
be self-imports.. but I guess no one has tried to run android cts on
panfrost).
What about something less drastic to start, like (apologies for
hand-edited patch):
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 86d670c71286..fc9ec42fa0ab 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object
*obj, unsigned long obj_size,
{
int ret;
+ WARN_ON_ONCE(obj->import_attach);
+
/* Check for valid size. */
if (obj_size < vma->vm_end - vma->vm_start)
return -EINVAL;
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 8ad0e02991ca..6190f5018986 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -609,17 +609,8 @@ EXPORT_SYMBOL_GPL(drm_gem_shmem_vm_ops);
*/
int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct
vm_area_struct *vma)
{
- struct drm_gem_object *obj = &shmem->base;
int ret;
if (obj->import_attach) {
- /* Drop the reference drm_gem_mmap_obj() acquired.*/
- drm_gem_object_put(obj);
- vma->vm_private_data = NULL;
-
- return dma_buf_mmap(obj->dma_buf, vma, 0);
+ return -EINVAL;
}
ret = drm_gem_shmem_get_pages(shmem);
if (ret) {
drm_gem_vm_close(vma);
--
2.36.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v8 2/2] drm/gem: Don't map imported GEMs
On Tue, Jul 5, 2022 at 4:51 AM Christian König wrote:
>
> Am 01.07.22 um 11:02 schrieb Dmitry Osipenko:
> > Drivers that use drm_gem_mmap() and drm_gem_mmap_obj() helpers don't
> > handle imported dma-bufs properly, which results in mapping of something
> > else than the imported dma-buf. On NVIDIA Tegra we get a hard lockup when
> > userspace writes to the memory mapping of a dma-buf that was imported into
> > Tegra's DRM GEM.
> >
> > Majority of DRM drivers prohibit mapping of the imported GEM objects.
> > Mapping of imported GEMs require special care from userspace since it
> > should sync dma-buf because mapping coherency of the exporter device may
> > not match the DRM device. Let's prohibit the mapping for all DRM drivers
> > for consistency.
> >
> > Suggested-by: Thomas Hellström
> > Signed-off-by: Dmitry Osipenko
>
> I'm pretty sure that this is the right approach, but it's certainly more
> than possible that somebody abused this already.
I suspect that this is abused if you run deqp cts on android.. ie. all
winsys buffers are dma-buf imports from gralloc. And then when you
hit readpix...
You might only hit this in scenarios with separate gpu and display (or
dGPU+iGPU) because self-imports are handled differently in
drm_gem_prime_import_dev().. and maybe not in cases where you end up
with a blit from tiled/compressed to linear.. maybe that narrows the
scope enough to just fix it in userspace?
BR,
-R
> Anyway patch is Reviewed-by: Christian König
> since you are really fixing a major stability problem here.
>
> Regards,
> Christian.
>
> > ---
> > drivers/gpu/drm/drm_gem.c | 4
> > drivers/gpu/drm/drm_gem_shmem_helper.c | 9 -
> > 2 files changed, 4 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 86d670c71286..fc9ec42fa0ab 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -1034,6 +1034,10 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj,
> > unsigned long obj_size,
> > {
> > int ret;
> >
> > + /* Don't allow imported objects to be mapped */
> > + if (obj->import_attach)
> > + return -EINVAL;
> > +
> > /* Check for valid size. */
> > if (obj_size < vma->vm_end - vma->vm_start)
> > return -EINVAL;
> > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > index 8ad0e02991ca..6190f5018986 100644
> > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > @@ -609,17 +609,8 @@ EXPORT_SYMBOL_GPL(drm_gem_shmem_vm_ops);
> >*/
> > int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct
> > vm_area_struct *vma)
> > {
> > - struct drm_gem_object *obj = &shmem->base;
> > int ret;
> >
> > - if (obj->import_attach) {
> > - /* Drop the reference drm_gem_mmap_obj() acquired.*/
> > - drm_gem_object_put(obj);
> > - vma->vm_private_data = NULL;
> > -
> > - return dma_buf_mmap(obj->dma_buf, vma, 0);
> > - }
> > -
> > ret = drm_gem_shmem_get_pages(shmem);
> > if (ret) {
> > drm_gem_vm_close(vma);
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v7 7/9] drm/virtio: Improve DMA API usage for shmem BOs
On Tue, Jul 5, 2022 at 10:02 AM Dmitry Osipenko wrote: > > On 7/5/22 18:45, Gerd Hoffmann wrote: > > Hi, > > > >>> Also note that pci is not the only virtio transport we have. > >> > >> The VirtIO indeed has other transports, but only PCI is really supported > >> in case of the VirtIO-GPU in kernel and in Qemu/crosvm, AFAICT. Hence > >> only the PCI transport was tested. > > > > qemu -M microvm \ > > -global virtio-mmio.force-legacy=false \ > > -device virtio-gpu-device > > > > Gives you a functional virtio-gpu device on virtio-mmio. > > > > aarch64 virt machines support both pci and mmio too. > > s390x has virtio-gpu-ccw ... > > Gerd, thank you very much! It's was indeed unclear to me how to test the > MMIO GPU, but yours variant with microvm works! I was looking for trying > aarch64 in the past, but it also was unclear how to do it since there is > no DT support for the VirtIO-GPU, AFAICS. just a drive-by note, IME on aarch64 kernels, at least with crosvm, virtgpu is also a pci device.. the non-pci things in the guest kernel use dt, but devices on discoverable busses like pci don't need dt nodes (which is true also in the non-vm case) BR, -R > I booted kernel with this patchset applied and everything is okay, Xorg > works. > > [drm] Initialized virtio_gpu 0.1.0 0 for LNRO0005:01 on minor 0 > virtio-mmio LNRO0005:01: [drm] drm_plane_enable_fb_damage_clips() not > called > virtio-mmio LNRO0005:01: [drm] fb0: virtio_gpudrmfb frame buffer device > > There is no virgl support because it's a virtio-gpu-device and not > virtio-gpu-device-gl that is PCI-only in Qemu. Hence everything seems good. > > I'd appreciate if you could give s390x a test.. I never touched s390x > and it will probably take some extra effort to get into it. > > -- > Best regards, > Dmitry ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v6 00/22] Add generic memory shrinker to VirtIO-GPU and Panfrost DRM drivers
On Tue, Jun 28, 2022 at 5:51 AM Dmitry Osipenko
wrote:
>
> On 6/28/22 15:31, Robin Murphy wrote:
> > ->8-
> > [ 68.295951] ==
> > [ 68.295956] WARNING: possible circular locking dependency detected
> > [ 68.295963] 5.19.0-rc3+ #400 Not tainted
> > [ 68.295972] --
> > [ 68.295977] cc1/295 is trying to acquire lock:
> > [ 68.295986] 08d7f1a0
> > (reservation_ww_class_mutex){+.+.}-{3:3}, at: drm_gem_shmem_free+0x7c/0x198
> > [ 68.296036]
> > [ 68.296036] but task is already holding lock:
> > [ 68.296041] 8c14b820 (fs_reclaim){+.+.}-{0:0}, at:
> > __alloc_pages_slowpath.constprop.0+0x4d8/0x1470
> > [ 68.296080]
> > [ 68.296080] which lock already depends on the new lock.
> > [ 68.296080]
> > [ 68.296085]
> > [ 68.296085] the existing dependency chain (in reverse order) is:
> > [ 68.296090]
> > [ 68.296090] -> #1 (fs_reclaim){+.+.}-{0:0}:
> > [ 68.296111]fs_reclaim_acquire+0xb8/0x150
> > [ 68.296130]dma_resv_lockdep+0x298/0x3fc
> > [ 68.296148]do_one_initcall+0xe4/0x5f8
> > [ 68.296163]kernel_init_freeable+0x414/0x49c
> > [ 68.296180]kernel_init+0x2c/0x148
> > [ 68.296195]ret_from_fork+0x10/0x20
> > [ 68.296207]
> > [ 68.296207] -> #0 (reservation_ww_class_mutex){+.+.}-{3:3}:
> > [ 68.296229]__lock_acquire+0x1724/0x2398
> > [ 68.296246]lock_acquire+0x218/0x5b0
> > [ 68.296260]__ww_mutex_lock.constprop.0+0x158/0x2378
> > [ 68.296277]ww_mutex_lock+0x7c/0x4d8
> > [ 68.296291]drm_gem_shmem_free+0x7c/0x198
> > [ 68.296304]panfrost_gem_free_object+0x118/0x138
> > [ 68.296318]drm_gem_object_free+0x40/0x68
> > [ 68.296334]drm_gem_shmem_shrinker_run_objects_scan+0x42c/0x5b8
> > [ 68.296352]drm_gem_shmem_shrinker_scan_objects+0xa4/0x170
> > [ 68.296368]do_shrink_slab+0x220/0x808
> > [ 68.296381]shrink_slab+0x11c/0x408
> > [ 68.296392]shrink_node+0x6ac/0xb90
> > [ 68.296403]do_try_to_free_pages+0x1dc/0x8d0
> > [ 68.296416]try_to_free_pages+0x1ec/0x5b0
> > [ 68.296429]__alloc_pages_slowpath.constprop.0+0x528/0x1470
> > [ 68.296444]__alloc_pages+0x4e0/0x5b8
> > [ 68.296455]__folio_alloc+0x24/0x60
> > [ 68.296467]vma_alloc_folio+0xb8/0x2f8
> > [ 68.296483]alloc_zeroed_user_highpage_movable+0x58/0x68
> > [ 68.296498]__handle_mm_fault+0x918/0x12a8
> > [ 68.296513]handle_mm_fault+0x130/0x300
> > [ 68.296527]do_page_fault+0x1d0/0x568
> > [ 68.296539]do_translation_fault+0xa0/0xb8
> > [ 68.296551]do_mem_abort+0x68/0xf8
> > [ 68.296562]el0_da+0x74/0x100
> > [ 68.296572]el0t_64_sync_handler+0x68/0xc0
> > [ 68.296585]el0t_64_sync+0x18c/0x190
> > [ 68.296596]
> > [ 68.296596] other info that might help us debug this:
> > [ 68.296596]
> > [ 68.296601] Possible unsafe locking scenario:
> > [ 68.296601]
> > [ 68.296604]CPU0CPU1
> > [ 68.296608]
> > [ 68.296612] lock(fs_reclaim);
> > [ 68.296622] lock(reservation_ww_class_mutex);
> > [ 68.296633]lock(fs_reclaim);
> > [ 68.296644] lock(reservation_ww_class_mutex);
> > [ 68.296654]
> > [ 68.296654] *** DEADLOCK ***
>
> This splat could be ignored for now. I'm aware about it, although
> haven't looked closely at how to fix it since it's a kind of a lockdep
> misreporting.
The lockdep splat could be fixed with something similar to what I've
done in msm, ie. basically just not acquire the lock in the finalizer:
https://patchwork.freedesktop.org/patch/489364/
There is one gotcha to watch for, as danvet pointed out
(scan_objects() could still see the obj in the LRU before the
finalizer removes it), but if scan_objects() does the
kref_get_unless_zero() trick, it is safe.
BR,
-R
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v6 17/22] drm/shmem-helper: Add generic memory shrinker
()
On Thu, May 26, 2022 at 4:55 PM Dmitry Osipenko
wrote:
>
> Introduce a common DRM SHMEM shrinker framework that allows to reduce
> code duplication among DRM drivers by replacing theirs custom shrinker
> implementations with the generic shrinker.
>
> In order to start using DRM SHMEM shrinker drivers should:
>
> 1. Implement new evict() shmem object callback.
> 2. Register shrinker using drm_gem_shmem_shrinker_register(drm_device).
> 3. Use drm_gem_shmem_set_purgeable(shmem) and alike API functions to
>activate shrinking of shmem GEMs.
>
> This patch is based on a ideas borrowed from Rob's Clark MSM shrinker,
> Thomas' Zimmermann variant of SHMEM shrinker and Intel's i915 shrinker.
>
> Signed-off-by: Daniel Almeida
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/gpu/drm/drm_gem_shmem_helper.c| 540 --
> .../gpu/drm/panfrost/panfrost_gem_shrinker.c | 9 +-
> drivers/gpu/drm/virtio/virtgpu_drv.h | 3 +
> include/drm/drm_device.h | 4 +
> include/drm/drm_gem_shmem_helper.h| 87 ++-
> 5 files changed, 594 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
> b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 555fe212bd98..4cd0b5913492 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -126,6 +126,42 @@ struct drm_gem_shmem_object *drm_gem_shmem_create(struct
> drm_device *dev, size_t
> }
> EXPORT_SYMBOL_GPL(drm_gem_shmem_create);
>
> +static bool drm_gem_shmem_is_evictable(struct drm_gem_shmem_object *shmem)
> +{
> + return (shmem->madv >= 0) && shmem->evict &&
> + shmem->eviction_enabled && shmem->pages_use_count &&
> + !shmem->pages_pin_count && !shmem->base.dma_buf &&
> + !shmem->base.import_attach && shmem->sgt && !shmem->evicted;
> +}
> +
> +static void
> +drm_gem_shmem_update_pages_state(struct drm_gem_shmem_object *shmem)
> +{
> + struct drm_gem_object *obj = &shmem->base;
> + struct drm_gem_shmem_shrinker *gem_shrinker =
> obj->dev->shmem_shrinker;
> +
> + dma_resv_assert_held(shmem->base.resv);
> +
> + if (!gem_shrinker || obj->import_attach)
> + return;
> +
> + mutex_lock(&gem_shrinker->lock);
> +
> + if (drm_gem_shmem_is_evictable(shmem) ||
> + drm_gem_shmem_is_purgeable(shmem))
> + list_move_tail(&shmem->madv_list,
> &gem_shrinker->lru_evictable);
> + else if (shmem->madv < 0)
> + list_del_init(&shmem->madv_list);
> + else if (shmem->evicted)
> + list_move_tail(&shmem->madv_list, &gem_shrinker->lru_evicted);
> + else if (!shmem->pages)
> + list_del_init(&shmem->madv_list);
> + else
> + list_move_tail(&shmem->madv_list, &gem_shrinker->lru_pinned);
> +
> + mutex_unlock(&gem_shrinker->lock);
> +}
> +
> /**
> * drm_gem_shmem_free - Free resources associated with a shmem GEM object
> * @shmem: shmem GEM object to free
> @@ -142,6 +178,9 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
> *shmem)
> } else {
> dma_resv_lock(shmem->base.resv, NULL);
>
> + /* take out shmem GEM object from the memory shrinker */
> + drm_gem_shmem_madvise(shmem, -1);
> +
> WARN_ON(shmem->vmap_use_count);
>
> if (shmem->sgt) {
> @@ -150,7 +189,7 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
> *shmem)
> sg_free_table(shmem->sgt);
> kfree(shmem->sgt);
> }
> - if (shmem->pages)
> + if (shmem->pages_use_count)
> drm_gem_shmem_put_pages(shmem);
>
> WARN_ON(shmem->pages_use_count);
> @@ -163,18 +202,82 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
> *shmem)
> }
> EXPORT_SYMBOL_GPL(drm_gem_shmem_free);
>
> -static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
> +/**
> + * drm_gem_shmem_set_evictable() - Make GEM evictable by memory shrinker
> + * @shmem: shmem GEM object
> + *
> + * Tell memory shrinker that this GEM can be evicted. Initially eviction is
> + * disabled for all GEMs. If GEM was purged, then -ENOMEM is returned.
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +int drm_gem_shmem_set_evictable(struct drm_gem_shmem_object *shmem)
> +{
> + dma_resv_lock(shmem->base.resv, NULL);
> +
> + if (shmem->madv < 0)
> + return -ENOMEM;
> +
> + shmem->eviction_enabled = true;
> +
> + dma_resv_unlock(shmem->base.resv);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_shmem_set_evictable);
> +
> +/**
> + * drm_gem_shmem_set_purgeable() - Make GEM purgeable by memory shrinker
> + * @shmem: shmem GEM object
> + *
> + * Tell memory shrinker that this GEM can be purged. Initially purging is
> + * disabled for al
Re: [PATCH v6 17/22] drm/shmem-helper: Add generic memory shrinker
On Mon, Jun 20, 2022 at 7:09 AM Dmitry Osipenko
wrote:
>
> On 6/19/22 20:53, Rob Clark wrote:
> ...
> >> +static unsigned long
> >> +drm_gem_shmem_shrinker_count_objects(struct shrinker *shrinker,
> >> +struct shrink_control *sc)
> >> +{
> >> + struct drm_gem_shmem_shrinker *gem_shrinker =
> >> to_drm_shrinker(shrinker);
> >> + struct drm_gem_shmem_object *shmem;
> >> + unsigned long count = 0;
> >> +
> >> + if (!mutex_trylock(&gem_shrinker->lock))
> >> + return 0;
> >> +
> >> + list_for_each_entry(shmem, &gem_shrinker->lru_evictable,
> >> madv_list) {
> >> + count += shmem->base.size;
> >> +
> >> + if (count >= SHRINK_EMPTY)
> >> + break;
> >> + }
> >> +
> >> + mutex_unlock(&gem_shrinker->lock);
> >
> > As I mentioned on other thread, count_objects, being approximate but
> > lockless and fast is the important thing. Otherwise when you start
> > hitting the shrinker on many threads, you end up serializing them all,
> > even if you have no pages to return to the system at that point.
>
> Daniel's point for dropping the lockless variant was that we're already
> in trouble if we're hitting shrinker too often and extra optimizations
> won't bring much benefits to us.
At least with zram swap (which I highly recommend using even if you
are not using a physical swap file/partition), swapin/out is actually
quite fast. And if you are leaning on zram swap to fit 8GB of chrome
browser on a 4GB device, the shrinker gets hit quite a lot. Lower
spec (4GB RAM) chromebooks can be under constant memory pressure and
can quite easily get into a situation where you are hitting the
shrinker on many threads simultaneously. So it is pretty important
for all shrinkers in the system (not just drm driver) to be as
concurrent as possible. As long as you avoid serializing reclaim on
all the threads, performance can still be quite good, but if you don't
performance will fall off a cliff.
jfwiw, we are seeing pretty good results (iirc 40-70% increase in open
tab counts) with the combination of eviction + multigen LRU[1] +
sizing zram swap to be 2x physical RAM
[1] https://lwn.net/Articles/856931/
> Alright, I'll add back the lockless variant (or will use yours
> drm_gem_lru) in the next revision. The code difference is very small
> after all.
>
> ...
> >> + /* prevent racing with the dma-buf importing/exporting */
> >> + if (!mutex_trylock(&gem_shrinker->dev->object_name_lock)) {
> >> + *lock_contention |= true;
> >> + goto resv_unlock;
> >> + }
> >
> > I'm not sure this is a good idea to serialize on object_name_lock.
> > Purgeable buffers should never be shared (imported or exported). So
> > at best you are avoiding evicting and immediately swapping back in, in
> > a rare case, at the cost of serializing multiple threads trying to
> > reclaim pages in parallel.
>
> The object_name_lock shouldn't cause contention in practice. But objects
> are also pinned on attachment, hence maybe this lock is indeed
> unnecessary.. I'll re-check it.
I'm not worried about contention with export/import/etc, but
contention between multiple threads hitting the shrinker in parallel.
I guess since you are using trylock, it won't *block* the other
threads hitting shrinker, but they'll just end up looping in
do_shrink_slab() because they are hitting contention.
I'd have to do some experiments to see how it works out in practice,
but my gut feel is that it isn't a good idea
BR,
-R
> --
> Best regards,
> Dmitry
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v6 17/22] drm/shmem-helper: Add generic memory shrinker
On Thu, May 26, 2022 at 4:55 PM Dmitry Osipenko
wrote:
>
> Introduce a common DRM SHMEM shrinker framework that allows to reduce
> code duplication among DRM drivers by replacing theirs custom shrinker
> implementations with the generic shrinker.
>
> In order to start using DRM SHMEM shrinker drivers should:
>
> 1. Implement new evict() shmem object callback.
> 2. Register shrinker using drm_gem_shmem_shrinker_register(drm_device).
> 3. Use drm_gem_shmem_set_purgeable(shmem) and alike API functions to
>activate shrinking of shmem GEMs.
>
> This patch is based on a ideas borrowed from Rob's Clark MSM shrinker,
> Thomas' Zimmermann variant of SHMEM shrinker and Intel's i915 shrinker.
>
> Signed-off-by: Daniel Almeida
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/gpu/drm/drm_gem_shmem_helper.c| 540 --
> .../gpu/drm/panfrost/panfrost_gem_shrinker.c | 9 +-
> drivers/gpu/drm/virtio/virtgpu_drv.h | 3 +
> include/drm/drm_device.h | 4 +
> include/drm/drm_gem_shmem_helper.h| 87 ++-
> 5 files changed, 594 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
> b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 555fe212bd98..4cd0b5913492 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -126,6 +126,42 @@ struct drm_gem_shmem_object *drm_gem_shmem_create(struct
> drm_device *dev, size_t
> }
> EXPORT_SYMBOL_GPL(drm_gem_shmem_create);
>
> +static bool drm_gem_shmem_is_evictable(struct drm_gem_shmem_object *shmem)
> +{
> + return (shmem->madv >= 0) && shmem->evict &&
> + shmem->eviction_enabled && shmem->pages_use_count &&
> + !shmem->pages_pin_count && !shmem->base.dma_buf &&
> + !shmem->base.import_attach && shmem->sgt && !shmem->evicted;
> +}
> +
> +static void
> +drm_gem_shmem_update_pages_state(struct drm_gem_shmem_object *shmem)
> +{
> + struct drm_gem_object *obj = &shmem->base;
> + struct drm_gem_shmem_shrinker *gem_shrinker =
> obj->dev->shmem_shrinker;
> +
> + dma_resv_assert_held(shmem->base.resv);
> +
> + if (!gem_shrinker || obj->import_attach)
> + return;
> +
> + mutex_lock(&gem_shrinker->lock);
> +
> + if (drm_gem_shmem_is_evictable(shmem) ||
> + drm_gem_shmem_is_purgeable(shmem))
> + list_move_tail(&shmem->madv_list,
> &gem_shrinker->lru_evictable);
> + else if (shmem->madv < 0)
> + list_del_init(&shmem->madv_list);
> + else if (shmem->evicted)
> + list_move_tail(&shmem->madv_list, &gem_shrinker->lru_evicted);
> + else if (!shmem->pages)
> + list_del_init(&shmem->madv_list);
> + else
> + list_move_tail(&shmem->madv_list, &gem_shrinker->lru_pinned);
> +
> + mutex_unlock(&gem_shrinker->lock);
> +}
> +
> /**
> * drm_gem_shmem_free - Free resources associated with a shmem GEM object
> * @shmem: shmem GEM object to free
> @@ -142,6 +178,9 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
> *shmem)
> } else {
> dma_resv_lock(shmem->base.resv, NULL);
>
> + /* take out shmem GEM object from the memory shrinker */
> + drm_gem_shmem_madvise(shmem, -1);
> +
> WARN_ON(shmem->vmap_use_count);
>
> if (shmem->sgt) {
> @@ -150,7 +189,7 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
> *shmem)
> sg_free_table(shmem->sgt);
> kfree(shmem->sgt);
> }
> - if (shmem->pages)
> + if (shmem->pages_use_count)
> drm_gem_shmem_put_pages(shmem);
>
> WARN_ON(shmem->pages_use_count);
> @@ -163,18 +202,82 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
> *shmem)
> }
> EXPORT_SYMBOL_GPL(drm_gem_shmem_free);
>
> -static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
> +/**
> + * drm_gem_shmem_set_evictable() - Make GEM evictable by memory shrinker
> + * @shmem: shmem GEM object
> + *
> + * Tell memory shrinker that this GEM can be evicted. Initially eviction is
> + * disabled for all GEMs. If GEM was purged, then -ENOMEM is returned.
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +int drm_gem_shmem_set_evictable(struct drm_gem_shmem_object *shmem)
> +{
> + dma_resv_lock(shmem->base.resv, NULL);
> +
> + if (shmem->madv < 0)
> + return -ENOMEM;
> +
> + shmem->eviction_enabled = true;
> +
> + dma_resv_unlock(shmem->base.resv);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_shmem_set_evictable);
> +
> +/**
> + * drm_gem_shmem_set_purgeable() - Make GEM purgeable by memory shrinker
> + * @shmem: shmem GEM object
> + *
> + * Tell memory shrinker that this GEM can be purged. Initially purging is
> + * disabled for all GE
Re: [PATCH v4 11/15] drm/shmem-helper: Add generic memory shrinker
e_count: > >>>> + * > >>>> + * The shmem pages are disallowed to be purged by the memory > >>>> shrinker > >>>> + * while count is non-zero. Used internally by memory shrinker. > >>>> + */ > >>>> +unsigned int purging_disable_count; > > > > What are these disable counts for? > > Some of BO types should stay pinned permanently, this applies to both > VirtIO and Panfrost drivers that make use of the generic shrinker in > this patchset. Hence I made objects unpurgeable and unevictable by default. > > Initially the idea of these counts was to allow drivers to explicitly > disable purging and eviction, and do it multiple times. If driver > disables eviction in two different places in the code, then we need to > track the eviction-disable count. > > In the v5 of this patchset drivers don't need to explicitly disable > shrinking anymore, they only need to enable it. The counts are also used > internally by DRM SHMEM core to track the vmappings and pinnings, but > perhaps pages_use_count could be used for that instead. I'll revisit it > for v6. > > > The way purgeable works in other drivers is that userspace sets purgeable > > or not, and it's up to userspace to not make a mess of this. > > > > There's also some interactions, and I guess a bunch of drivers get this > > wrong in funny ways. Not sure how to best clean this up. > > > > - Once you have a shrinker/dynamic memory management you should _not_ pin > > pages, except when it's truly permanent like for scanout. Instead > > drivers should attach dma_fence to the dma_resv to denote in-flight > > access. > > By default pages are pinned when drm_gem_shmem_get_pages_sgt() is > invoked by drivers during of BO creation time. > > We could declare that pages_use_count=1 means the pages are allowed to > be evicted and purged if shrinker is enabled. Then the further > drm_gem_shmem_pin/vmap() calls will bump the pages_use_count, > disallowing the eviction and purging, like you're suggesting, and we > won't need the explicit counts. > > > - A pinned buffer object is not allowed to be put into purgeable state, > > and a bo in purgeable state should not be allowed to be pinned. > > > > - Drivers need to hold dma_resv_lock for long enough in their command > > submission, i.e. from the point where the reserve the buffers and make > > sure that mappings exists, to the point where the request is submitted > > to hw or drm/sched and fences are installed. > > > > But I think a lot of current shmem users just pin as part of execbuf, so > > this won't work quite so well right out of the box. > > The current shmem users assume that BO is pinned permanently once it has > been created. > > > Anyway with that design I don't think there should ever be a need to > > disable shrinking. > > To me what you described mostly matches to what I did in the v5. > > >>>> + > >>>> +/** > >>>> + * @pages_state: Current state of shmem pages. Used internally by > >>>> + * memory shrinker. > >>>> + */ > >>>> +enum drm_gem_shmem_pages_state pages_state; > >>>> + > >>>> +/** > >>>> + * @evicted: True if shmem pages were evicted by the memory > >>>> shrinker. > >>>> + * Used internally by memory shrinker. > >>>> + */ > >>>> +bool evicted; > >>>> + > >>>> +/** > >>>> + * @pages_shrinkable: True if shmem pages can be evicted or purged > >>>> + * by the memory shrinker. Used internally by memory shrinker. > >>>> + */ > >>>> +bool pages_shrinkable; > >>> > >>> As commented before, this state can be foundby looking at existing > >>> fields. No need to store it separately. > >> > >> When we're transitioning from "evictable" to a "purgeable" state, we > >> must not add pages twice to the "shrinkable_count" variable. Hence this > >> is not a state, but a variable which prevents the double accounting of > >> the pages. Please see drm_gem_shmem_add_pages_to_shrinker() in this patch. > >> > >> Perhaps something like "pages_accounted_by_shrinker" could be a better > >> name for the variable. I'll revisit this for v5. > > > > Hm not sure we need to account this? Usually the shrinker just counts when > > it's asked to do so, not practively maintain that count. Once you start > > shrinking burning cpu time is generally not too terrible. > > We could count pages on demand by walking up the "evictable" list, but > then the shrinker's lock needs to be taken by the > drm_gem_shmem_shrinker_count_objects() to protect the list. > > Previously Rob Clark said that the profiling of freedreno's shrinker > showed that it's worthwhile to reduce the locks as much as possible, > including the case of counting shrinkable objects. Sorry I missed this earlier, but danvet is giving some bad advice here ;-) You *really* need count_objects() to be lockless and fast, ie. no list iteration. It doesn't have to return the "perfect" value, so it is ok if it is racy / not-atomic / etc. Otherwise you will have bad system performance issues when you start hitting do_shrink_slab() on many threads at once. BR, -R ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v6 17/22] drm/shmem-helper: Add generic memory shrinker
On Sun, Jun 5, 2022 at 9:47 AM Daniel Vetter wrote:
>
> On Fri, 27 May 2022 at 01:55, Dmitry Osipenko
> wrote:
> >
> > Introduce a common DRM SHMEM shrinker framework that allows to reduce
> > code duplication among DRM drivers by replacing theirs custom shrinker
> > implementations with the generic shrinker.
> >
> > In order to start using DRM SHMEM shrinker drivers should:
> >
> > 1. Implement new evict() shmem object callback.
> > 2. Register shrinker using drm_gem_shmem_shrinker_register(drm_device).
> > 3. Use drm_gem_shmem_set_purgeable(shmem) and alike API functions to
> >activate shrinking of shmem GEMs.
> >
> > This patch is based on a ideas borrowed from Rob's Clark MSM shrinker,
> > Thomas' Zimmermann variant of SHMEM shrinker and Intel's i915 shrinker.
> >
> > Signed-off-by: Daniel Almeida
> > Signed-off-by: Dmitry Osipenko
>
> So I guess I get a price for being blind since forever, because this
> thing existed since at least 2013. I just stumbled over
> llist_lru.[hc], a purpose built list helper for shrinkers. I think we
> should try to adopt that so that our gpu shrinkers look more like
> shrinkers for everything else.
followup from a bit of irc discussion w/ danvet about list_lru:
* It seems to be missing a way to bail out of iteration before
nr_to_scan is hit.. which is going to be inconvenient if you
want to allow active bos on the LRU but bail scanning once
you encounter the first one.
* Not sure if the numa node awareness is super useful for GEM
bos
First issue is perhaps not too hard to fix. But maybe a better
idea is a drm_gem_lru helper type thing which is more tailored
to GEM buffers?
BR,
-R
> Apologies for this, since I fear this might cause a bit of churn.
> Hopefully it's all contained to the list manipulation code in shmem
> helpers, I don't think this should leak any further.
> -Daniel
>
> > ---
> > drivers/gpu/drm/drm_gem_shmem_helper.c| 540 --
> > .../gpu/drm/panfrost/panfrost_gem_shrinker.c | 9 +-
> > drivers/gpu/drm/virtio/virtgpu_drv.h | 3 +
> > include/drm/drm_device.h | 4 +
> > include/drm/drm_gem_shmem_helper.h| 87 ++-
> > 5 files changed, 594 insertions(+), 49 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > index 555fe212bd98..4cd0b5913492 100644
> > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > @@ -126,6 +126,42 @@ struct drm_gem_shmem_object
> > *drm_gem_shmem_create(struct drm_device *dev, size_t
> > }
> > EXPORT_SYMBOL_GPL(drm_gem_shmem_create);
> >
> > +static bool drm_gem_shmem_is_evictable(struct drm_gem_shmem_object *shmem)
> > +{
> > + return (shmem->madv >= 0) && shmem->evict &&
> > + shmem->eviction_enabled && shmem->pages_use_count &&
> > + !shmem->pages_pin_count && !shmem->base.dma_buf &&
> > + !shmem->base.import_attach && shmem->sgt && !shmem->evicted;
> > +}
> > +
> > +static void
> > +drm_gem_shmem_update_pages_state(struct drm_gem_shmem_object *shmem)
> > +{
> > + struct drm_gem_object *obj = &shmem->base;
> > + struct drm_gem_shmem_shrinker *gem_shrinker =
> > obj->dev->shmem_shrinker;
> > +
> > + dma_resv_assert_held(shmem->base.resv);
> > +
> > + if (!gem_shrinker || obj->import_attach)
> > + return;
> > +
> > + mutex_lock(&gem_shrinker->lock);
> > +
> > + if (drm_gem_shmem_is_evictable(shmem) ||
> > + drm_gem_shmem_is_purgeable(shmem))
> > + list_move_tail(&shmem->madv_list,
> > &gem_shrinker->lru_evictable);
> > + else if (shmem->madv < 0)
> > + list_del_init(&shmem->madv_list);
> > + else if (shmem->evicted)
> > + list_move_tail(&shmem->madv_list,
> > &gem_shrinker->lru_evicted);
> > + else if (!shmem->pages)
> > + list_del_init(&shmem->madv_list);
> > + else
> > + list_move_tail(&shmem->madv_list,
> > &gem_shrinker->lru_pinned);
> > +
> > + mutex_unlock(&gem_shrinker->lock);
> > +}
> > +
> > /**
> > * drm_gem_shmem_free - Free resources associated with a shmem GEM object
> > * @shmem: shmem GEM object to free
> > @@ -142,6 +178,9 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
> > *shmem)
> > } else {
> > dma_resv_lock(shmem->base.resv, NULL);
> >
> > + /* take out shmem GEM object from the memory shrinker */
> > + drm_gem_shmem_madvise(shmem, -1);
> > +
> > WARN_ON(shmem->vmap_use_count);
> >
> > if (shmem->sgt) {
> > @@ -150,7 +189,7 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
> > *shmem)
> > sg_free_table(shmem->sgt);
> > kfree(shmem->sgt);
> > }
> > - if (shmem->pages)
> > + if (shmem->pages_use_count)
> >
Re: [PATCH] drm/virtio: Add execbuf flag to request no fence-event
On Tue, Apr 5, 2022 at 10:57 AM Chia-I Wu wrote:
>
> On Tue, Apr 5, 2022 at 10:38 AM Rob Clark wrote:
> >
> > From: Rob Clark
> >
> > It would have been cleaner to have a flag to *request* the fence event.
> > But that ship has sailed. So add a flag so that userspace which doesn't
> > care about the events can opt-out.
> >
> > Signed-off-by: Rob Clark
> Reviewed-by: Chia-I Wu
>
> Might want to wait for Gurchetan to chime in as he added the mechanism.
It turns out this patch is unnecessary.. I can simply not set
VIRTGPU_CONTEXT_PARAM_POLL_RINGS_MASK instead
so self-nak for this patch ;-)
BR,
-R
> > ---
> > drivers/gpu/drm/virtio/virtgpu_ioctl.c | 8 +---
> > include/uapi/drm/virtgpu_drm.h | 2 ++
> > 2 files changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> > b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> > index 3a8078f2ee27..09f1aa263f91 100644
> > --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> > +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> > @@ -225,9 +225,11 @@ static int virtio_gpu_execbuffer_ioctl(struct
> > drm_device *dev, void *data,
> > goto out_unresv;
> > }
> >
> > - ret = virtio_gpu_fence_event_create(dev, file, out_fence, ring_idx);
> > - if (ret)
> > - goto out_unresv;
> > + if (!(exbuf->flags & VIRTGPU_EXECBUF_NO_EVENT)) {
> > + ret = virtio_gpu_fence_event_create(dev, file, out_fence,
> > ring_idx);
> > + if (ret)
> > + goto out_unresv;
> > + }
> >
> > if (out_fence_fd >= 0) {
> > sync_file = sync_file_create(&out_fence->f);
> > diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
> > index 0512fde5e697..d06cac3407cc 100644
> > --- a/include/uapi/drm/virtgpu_drm.h
> > +++ b/include/uapi/drm/virtgpu_drm.h
> > @@ -52,10 +52,12 @@ extern "C" {
> > #define VIRTGPU_EXECBUF_FENCE_FD_IN0x01
> > #define VIRTGPU_EXECBUF_FENCE_FD_OUT 0x02
> > #define VIRTGPU_EXECBUF_RING_IDX 0x04
> > +#define VIRTGPU_EXECBUF_NO_EVENT 0x08
> > #define VIRTGPU_EXECBUF_FLAGS (\
> > VIRTGPU_EXECBUF_FENCE_FD_IN |\
> > VIRTGPU_EXECBUF_FENCE_FD_OUT |\
> > VIRTGPU_EXECBUF_RING_IDX |\
> > + VIRTGPU_EXECBUF_NO_EVENT |\
> > 0)
> >
> > struct drm_virtgpu_map {
> > --
> > 2.35.1
> >
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] drm/virtio: Add execbuf flag to request no fence-event
From: Rob Clark
It would have been cleaner to have a flag to *request* the fence event.
But that ship has sailed. So add a flag so that userspace which doesn't
care about the events can opt-out.
Signed-off-by: Rob Clark
---
drivers/gpu/drm/virtio/virtgpu_ioctl.c | 8 +---
include/uapi/drm/virtgpu_drm.h | 2 ++
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index 3a8078f2ee27..09f1aa263f91 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -225,9 +225,11 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device
*dev, void *data,
goto out_unresv;
}
- ret = virtio_gpu_fence_event_create(dev, file, out_fence, ring_idx);
- if (ret)
- goto out_unresv;
+ if (!(exbuf->flags & VIRTGPU_EXECBUF_NO_EVENT)) {
+ ret = virtio_gpu_fence_event_create(dev, file, out_fence,
ring_idx);
+ if (ret)
+ goto out_unresv;
+ }
if (out_fence_fd >= 0) {
sync_file = sync_file_create(&out_fence->f);
diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
index 0512fde5e697..d06cac3407cc 100644
--- a/include/uapi/drm/virtgpu_drm.h
+++ b/include/uapi/drm/virtgpu_drm.h
@@ -52,10 +52,12 @@ extern "C" {
#define VIRTGPU_EXECBUF_FENCE_FD_IN0x01
#define VIRTGPU_EXECBUF_FENCE_FD_OUT 0x02
#define VIRTGPU_EXECBUF_RING_IDX 0x04
+#define VIRTGPU_EXECBUF_NO_EVENT 0x08
#define VIRTGPU_EXECBUF_FLAGS (\
VIRTGPU_EXECBUF_FENCE_FD_IN |\
VIRTGPU_EXECBUF_FENCE_FD_OUT |\
VIRTGPU_EXECBUF_RING_IDX |\
+ VIRTGPU_EXECBUF_NO_EVENT |\
0)
struct drm_virtgpu_map {
--
2.35.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 6/8] drm/shmem-helper: Add generic memory shrinker
On Wed, Mar 16, 2022 at 5:13 PM Dmitry Osipenko
wrote:
>
> On 3/16/22 23:00, Rob Clark wrote:
> > On Mon, Mar 14, 2022 at 3:44 PM Dmitry Osipenko
> > wrote:
> >>
> >> Introduce a common DRM SHMEM shrinker. It allows to reduce code
> >> duplication among DRM drivers, it also handles complicated lockings
> >> for the drivers. This is initial version of the shrinker that covers
> >> basic needs of GPU drivers.
> >>
> >> This patch is based on a couple ideas borrowed from Rob's Clark MSM
> >> shrinker and Thomas' Zimmermann variant of SHMEM shrinker.
> >>
> >> GPU drivers that want to use generic DRM memory shrinker must support
> >> generic GEM reservations.
> >>
> >> Signed-off-by: Daniel Almeida
> >> Signed-off-by: Dmitry Osipenko
> >> ---
> >> drivers/gpu/drm/drm_gem_shmem_helper.c | 194 +
> >> include/drm/drm_device.h | 4 +
> >> include/drm/drm_gem.h | 11 ++
> >> include/drm/drm_gem_shmem_helper.h | 25
> >> 4 files changed, 234 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
> >> b/drivers/gpu/drm/drm_gem_shmem_helper.c
> >> index 37009418cd28..35be2ee98f11 100644
> >> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> >> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> >> @@ -139,6 +139,9 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
> >> *shmem)
> >> {
> >> struct drm_gem_object *obj = &shmem->base;
> >>
> >> + /* take out shmem GEM object from the memory shrinker */
> >> + drm_gem_shmem_madvise(shmem, 0);
> >> +
> >> WARN_ON(shmem->vmap_use_count);
> >>
> >> if (obj->import_attach) {
> >> @@ -163,6 +166,42 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
> >> *shmem)
> >> }
> >> EXPORT_SYMBOL_GPL(drm_gem_shmem_free);
> >>
> >> +static void drm_gem_shmem_update_purgeable_status(struct
> >> drm_gem_shmem_object *shmem)
> >> +{
> >> + struct drm_gem_object *obj = &shmem->base;
> >> + struct drm_gem_shmem_shrinker *gem_shrinker =
> >> obj->dev->shmem_shrinker;
> >> + size_t page_count = obj->size >> PAGE_SHIFT;
> >> +
> >> + if (!gem_shrinker || obj->import_attach || !obj->funcs->purge)
> >> + return;
> >> +
> >> + mutex_lock(&shmem->vmap_lock);
> >> + mutex_lock(&shmem->pages_lock);
> >> + mutex_lock(&gem_shrinker->lock);
> >> +
> >> + if (shmem->madv < 0) {
> >> + list_del_init(&shmem->madv_list);
> >> + goto unlock;
> >> + } else if (shmem->madv > 0) {
> >> + if (!list_empty(&shmem->madv_list))
> >> + goto unlock;
> >> +
> >> + WARN_ON(gem_shrinker->shrinkable_count + page_count <
> >> page_count);
> >> + gem_shrinker->shrinkable_count += page_count;
> >> +
> >> + list_add_tail(&shmem->madv_list, &gem_shrinker->lru);
> >> + } else if (!list_empty(&shmem->madv_list)) {
> >> + list_del_init(&shmem->madv_list);
> >> +
> >> + WARN_ON(gem_shrinker->shrinkable_count < page_count);
> >> + gem_shrinker->shrinkable_count -= page_count;
> >> + }
> >> +unlock:
> >> + mutex_unlock(&gem_shrinker->lock);
> >> + mutex_unlock(&shmem->pages_lock);
> >> + mutex_unlock(&shmem->vmap_lock);
> >> +}
> >> +
> >> static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object
> >> *shmem)
> >> {
> >> struct drm_gem_object *obj = &shmem->base;
> >> @@ -366,6 +405,8 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object
> >> *shmem,
> >> ret = drm_gem_shmem_vmap_locked(shmem, map);
> >> mutex_unlock(&shmem->vmap_lock);
> >>
> >> + drm_gem_shmem_update_purgeable_status(shmem);
> >> +
> >> return ret;
> >> }
> >> EXPORT_SYMBOL(drm_gem_shmem_vmap);
> >> @@ -409,6 +450,8 @@ void drm_gem_shmem_vunmap(struct drm_gem_s
Re: [PATCH v2 6/8] drm/shmem-helper: Add generic memory shrinker
On Mon, Mar 14, 2022 at 3:44 PM Dmitry Osipenko
wrote:
>
> Introduce a common DRM SHMEM shrinker. It allows to reduce code
> duplication among DRM drivers, it also handles complicated lockings
> for the drivers. This is initial version of the shrinker that covers
> basic needs of GPU drivers.
>
> This patch is based on a couple ideas borrowed from Rob's Clark MSM
> shrinker and Thomas' Zimmermann variant of SHMEM shrinker.
>
> GPU drivers that want to use generic DRM memory shrinker must support
> generic GEM reservations.
>
> Signed-off-by: Daniel Almeida
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/gpu/drm/drm_gem_shmem_helper.c | 194 +
> include/drm/drm_device.h | 4 +
> include/drm/drm_gem.h | 11 ++
> include/drm/drm_gem_shmem_helper.h | 25
> 4 files changed, 234 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
> b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 37009418cd28..35be2ee98f11 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -139,6 +139,9 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
> *shmem)
> {
> struct drm_gem_object *obj = &shmem->base;
>
> + /* take out shmem GEM object from the memory shrinker */
> + drm_gem_shmem_madvise(shmem, 0);
> +
> WARN_ON(shmem->vmap_use_count);
>
> if (obj->import_attach) {
> @@ -163,6 +166,42 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
> *shmem)
> }
> EXPORT_SYMBOL_GPL(drm_gem_shmem_free);
>
> +static void drm_gem_shmem_update_purgeable_status(struct
> drm_gem_shmem_object *shmem)
> +{
> + struct drm_gem_object *obj = &shmem->base;
> + struct drm_gem_shmem_shrinker *gem_shrinker =
> obj->dev->shmem_shrinker;
> + size_t page_count = obj->size >> PAGE_SHIFT;
> +
> + if (!gem_shrinker || obj->import_attach || !obj->funcs->purge)
> + return;
> +
> + mutex_lock(&shmem->vmap_lock);
> + mutex_lock(&shmem->pages_lock);
> + mutex_lock(&gem_shrinker->lock);
> +
> + if (shmem->madv < 0) {
> + list_del_init(&shmem->madv_list);
> + goto unlock;
> + } else if (shmem->madv > 0) {
> + if (!list_empty(&shmem->madv_list))
> + goto unlock;
> +
> + WARN_ON(gem_shrinker->shrinkable_count + page_count <
> page_count);
> + gem_shrinker->shrinkable_count += page_count;
> +
> + list_add_tail(&shmem->madv_list, &gem_shrinker->lru);
> + } else if (!list_empty(&shmem->madv_list)) {
> + list_del_init(&shmem->madv_list);
> +
> + WARN_ON(gem_shrinker->shrinkable_count < page_count);
> + gem_shrinker->shrinkable_count -= page_count;
> + }
> +unlock:
> + mutex_unlock(&gem_shrinker->lock);
> + mutex_unlock(&shmem->pages_lock);
> + mutex_unlock(&shmem->vmap_lock);
> +}
> +
> static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem)
> {
> struct drm_gem_object *obj = &shmem->base;
> @@ -366,6 +405,8 @@ int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
> ret = drm_gem_shmem_vmap_locked(shmem, map);
> mutex_unlock(&shmem->vmap_lock);
>
> + drm_gem_shmem_update_purgeable_status(shmem);
> +
> return ret;
> }
> EXPORT_SYMBOL(drm_gem_shmem_vmap);
> @@ -409,6 +450,8 @@ void drm_gem_shmem_vunmap(struct drm_gem_shmem_object
> *shmem,
> mutex_lock(&shmem->vmap_lock);
> drm_gem_shmem_vunmap_locked(shmem, map);
> mutex_unlock(&shmem->vmap_lock);
> +
> + drm_gem_shmem_update_purgeable_status(shmem);
> }
> EXPORT_SYMBOL(drm_gem_shmem_vunmap);
>
> @@ -451,6 +494,8 @@ int drm_gem_shmem_madvise(struct drm_gem_shmem_object
> *shmem, int madv)
>
> mutex_unlock(&shmem->pages_lock);
>
> + drm_gem_shmem_update_purgeable_status(shmem);
> +
> return (madv >= 0);
> }
> EXPORT_SYMBOL(drm_gem_shmem_madvise);
> @@ -763,6 +808,155 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device
> *dev,
> }
> EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_sg_table);
>
> +static struct drm_gem_shmem_shrinker *
> +to_drm_shrinker(struct shrinker *shrinker)
> +{
> + return container_of(shrinker, struct drm_gem_shmem_shrinker, base);
> +}
> +
> +static unsigned long
> +drm_gem_shmem_shrinker_count_objects(struct shrinker *shrinker,
> +struct shrink_control *sc)
> +{
> + struct drm_gem_shmem_shrinker *gem_shrinker =
> to_drm_shrinker(shrinker);
> + u64 count = gem_shrinker->shrinkable_count;
> +
> + if (count >= SHRINK_EMPTY)
> + return SHRINK_EMPTY - 1;
> +
> + return count ?: SHRINK_EMPTY;
> +}
> +
> +static unsigned long
> +drm_gem_shmem_shrinker_scan_objects(struct shrinker *shrinker,
> + struct shrink_control *sc)
> +{
> + st
Re: [PATCH v1 0/5] Add memory shrinker to VirtIO-GPU DRM driver
On Wed, Mar 9, 2022 at 12:06 PM Dmitry Osipenko wrote: > > On 3/9/22 03:56, Rob Clark wrote: > >> If we really can't track madvise state in the guest for dealing with > >> host memory pressure, I think the better option is to introduce > >> MADV:WILLNEED_REPLACE, ie. something to tell the host kernel that the > >> buffer is needed but the previous contents are not (as long as the GPU > >> VA remains the same). With this the host could allocate new pages if > >> needed, and the guest would not need to wait for a reply from host. > > If variant with the memory ballooning will work, then it will be > > possible to track the state within guest-only. Let's consider the > > simplest variant for now. > > > > I'll try to implement the balloon driver support in the v2 and will get > > back to you. > > > > I looked at the generic balloon driver and looks like this not what we > want because: > > 1. Memory ballooning is primarily about handling memory overcommit > situations. I.e. when there are multiple VMs consuming more memory than > available in the system. Ballooning allows host to ask guest to give > unused pages back to host and host could give pages to other VMs. > > 2. Memory ballooning operates with guest memory pages only. I.e. each > ballooned page is reported to/from host in a form of page's DMA address. > > 3. There is no direct connection between host's OOM events and the > balloon manager. I guess host could watch system's memory pressure and > inflate VMs' balloons on low memory, releasing the guest's memory to the > system, but apparently this use-case not supported by anyone today, at > least I don't see Qemu supporting it. > hmm, on CrOS I do see balloon getting used to balance host vs guest memory.. but admittedly I've not yet looked closely at how that works, and it does seem like we have some things that are not yet upstream all over the place (not to mention crosvm vs qemu) > > So the virtio-balloon driver isn't very useful for us as-is. > > One possible solution could be to create something like a new > virtio-shrinker device or add shrinker functionality to the virtio-gpu > device, allowing host to ask guests to drop shared caches. Host then > should become a PSI handler. I think this should be doable in a case of > crosvm. In a case of GNU world, it could take a lot of effort to get > everything to upstreamable state, at first there is a need to > demonstrate real problem being solved by this solution. I guess with 4GB chromebooks running one or more VMs in addition to lots of browser tabs in the host, it shouldn't be too hard to demonstrate a problem ;-) (but also, however we end up solving that, certainly shouldn't block this series) > The other minor issue is that only integrated GPUs may use system's > memory and even then they could use a dedicated memory carveout, i.e. > releasing VRAM BOs may not help with host's OOM. In case of virgl > context we have no clue about where buffers are physically located. On > the other hand, in the worst case dropping host caches just won't help > with OOM. Userspace should know whether the BO has CPU storage, so I don't think this should be a problem virtio_gpu needs to worry about > It's now unclear how we should proceed with the host-side shrinker > support. Thoughts? > > We may start easy and instead of thinking about host-side shrinker, we > could make VirtIO-GPU driver to expire cached BOs after a certain > timeout. Mesa already uses timeout-based BO caching, but it doesn't have > an alarm timer and simply checks expiration when BO is allocated. Should > be too much trouble to handle timers within Mesa since it's executed in > application context, easier to do it in kernel, like VC4 driver does it > for example. This is not good as a proper memory shrinker, but could be > good enough in practice. I think that, given virgl uses host storage, guest shrinker should be still useful.. so I think continue with this series. For host shrinker, I'll have to look more at when crosvm triggers balloon inflation. I could still go the MADV:WILLNEED_REPLACE approach instead, which does have the advantage of host kernel not relying on host userspace or vm having a chance to run in order to release pages. The downside (perhaps?) is it would be more specific to virtgpu-native-context and less so to virgl or venus (but I guess there doesn't currently exist a way for madvise to be useful for vk drivers). BR, -R ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v1 5/5] drm/virtio: Add memory shrinker
On Tue, Mar 8, 2022 at 5:17 AM Dmitry Osipenko
wrote:
>
> Add memory shrinker and new madvise IOCTL to the VirtIO-GPU driver.
> Userspace (BO cache manager of Mesa driver) will mark BOs as "don't need"
> using the new IOCTL to let shrinker purge the marked BOs on OOM, thus
> shrinker will lower memory pressure and prevent OOM kills.
>
> Signed-off-by: Daniel Almeida
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/gpu/drm/virtio/Makefile | 3 +-
> drivers/gpu/drm/virtio/virtgpu_drv.h | 26 +++-
> drivers/gpu/drm/virtio/virtgpu_gem.c | 84
> drivers/gpu/drm/virtio/virtgpu_gem_shrinker.c | 124 ++
> drivers/gpu/drm/virtio/virtgpu_ioctl.c| 37 ++
> drivers/gpu/drm/virtio/virtgpu_kms.c | 10 ++
> drivers/gpu/drm/virtio/virtgpu_object.c | 7 +
> drivers/gpu/drm/virtio/virtgpu_plane.c| 17 ++-
> drivers/gpu/drm/virtio/virtgpu_vq.c | 15 +++
> include/uapi/drm/virtgpu_drm.h| 14 ++
> 10 files changed, 333 insertions(+), 4 deletions(-)
> create mode 100644 drivers/gpu/drm/virtio/virtgpu_gem_shrinker.c
>
[snip]
> diff --git a/drivers/gpu/drm/virtio/virtgpu_gem_shrinker.c
> b/drivers/gpu/drm/virtio/virtgpu_gem_shrinker.c
> new file mode 100644
> index ..39eb9a3e7e4a
> --- /dev/null
> +++ b/drivers/gpu/drm/virtio/virtgpu_gem_shrinker.c
> @@ -0,0 +1,124 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2022 Collabora Ltd.
> + */
> +
> +#include
> +#include
> +
> +#include "virtgpu_drv.h"
> +
> +static unsigned long
> +virtio_gpu_gem_shrinker_count_objects(struct shrinker *shrinker,
> + struct shrink_control *sc)
> +{
> + struct drm_gem_shmem_object *shmem;
> + struct virtio_gpu_device *vgdev;
> + unsigned long count = 0;
> + bool empty = true;
> +
> + vgdev = container_of(shrinker, struct virtio_gpu_device,
> +vgshrinker.shrinker);
> +
> + if (!mutex_trylock(&vgdev->mm_lock))
> + return 0;
One bit of advice from previously dealing with shrinker and heavy
memory pressure situations (turns out 4GB chromebooks can be pretty
much under *constant* memory pressure):
You *really* want to make shrinker->count_objects lockless.. and
minimize the lock contention on shrinker->scan_objects (ie. The
problem is you can end up with shrinking going on on all CPU cores in
parallel, you want to not funnel that thru one lock as much as
possible.
See in particular:
25ed38b3ed26 ("drm/msm: Drop mm_lock in scan loop")
cc8a4d5a1bd8 ("drm/msm: Avoid mutex in shrinker_count()")
BR,
-R
> + list_for_each_entry(shmem, &vgdev->vgshrinker.list, madv_list) {
> + empty = false;
> +
> + if (!mutex_trylock(&shmem->pages_lock))
> + continue;
> +
> + if (drm_gem_shmem_is_purgeable(shmem))
> + count += shmem->base.size >> PAGE_SHIFT;
> +
> + mutex_unlock(&shmem->pages_lock);
> + }
> +
> + mutex_unlock(&vgdev->mm_lock);
> +
> + return empty ? SHRINK_EMPTY : count;
> +}
> +
> +static bool virtio_gpu_gem_shrinker_purge(struct virtio_gpu_device *vgdev,
> + struct drm_gem_object *obj)
> +{
> + struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
> + struct drm_gem_shmem_object *shmem = &bo->base;
> + int err;
> +
> + if (!dma_resv_test_signaled(obj->resv, true) ||
> + !drm_gem_shmem_is_purgeable(shmem) ||
> + refcount_read(&bo->pin_count))
> + return false;
> +
> + /*
> +* Release host's memory before guest's memory is gone to ensure that
> +* host won't touch released memory of the guest.
> +*/
> + err = virtio_gpu_gem_host_mem_release(bo);
> + if (err)
> + return false;
> +
> + list_del_init(&shmem->madv_list);
> + drm_gem_shmem_purge_locked(shmem);
> +
> + return true;
> +}
> +
> +static unsigned long
> +virtio_gpu_gem_shrinker_scan_objects(struct shrinker *shrinker,
> +struct shrink_control *sc)
> +{
> + struct drm_gem_shmem_object *shmem, *tmp;
> + struct virtio_gpu_device *vgdev;
> + unsigned long freed = 0;
> +
> + vgdev = container_of(shrinker, struct virtio_gpu_device,
> +vgshrinker.shrinker);
> +
> + if (!mutex_trylock(&vgdev->mm_lock))
> + return SHRINK_STOP;
> +
> + list_for_each_entry_safe(shmem, tmp, &vgdev->vgshrinker.list,
> madv_list) {
> + if (freed >= sc->nr_to_scan)
> + break;
> +
> + if (!dma_resv_trylock(shmem->base.resv))
> + continue;
> +
> + if (!mutex_trylock(&shmem->pages_lock))
> + goto resv_unlock;
> +
> + if
Re: [PATCH v1 0/5] Add memory shrinker to VirtIO-GPU DRM driver
On Tue, Mar 8, 2022 at 3:36 PM Dmitry Osipenko wrote: > > On 3/9/22 01:24, Rob Clark wrote: > > On Tue, Mar 8, 2022 at 11:28 AM Dmitry Osipenko > > wrote: > >> > >> On 3/8/22 19:29, Rob Clark wrote: > >>> On Tue, Mar 8, 2022 at 5:17 AM Dmitry Osipenko > >>> wrote: > >>>> > >>>> Hello, > >>>> > >>>> This patchset introduces memory shrinker for the VirtIO-GPU DRM driver. > >>>> During OOM, the shrinker will release BOs that are marked as "not needed" > >>>> by userspace using the new madvise IOCTL. The userspace in this case is > >>>> the Mesa VirGL driver, it will mark the cached BOs as "not needed", > >>>> allowing kernel driver to release memory of the cached shmem BOs on > >>>> lowmem > >>>> situations, preventing OOM kills. > >>> > >>> Will host memory pressure already trigger shrinker in guest? > >> > >> The host memory pressure won't trigger shrinker in guest here. This > >> series will help only with the memory pressure within the guest using a > >> usual "virgl context". > >> > >> Having a host shrinker in a case of "virgl contexts" should be a > >> difficult problem to solve. > > > > Hmm, I think we just need the balloon driver to trigger the shrinker > > in the guest kernel? I suppose a driver like drm/virtio might want to > > differentiate between host and guest pressure (ie. consider only > > objects that have host vs guest storage), but even without that, > > freeing up memory in the guest when host is under memory pressure > > seems worthwhile. Maybe I'm over-simplifying? > > Might be the opposite, i.e. me over-complicating :) The variant with > memory ballooning actually could be good and will work for all kinds of > virtio contexts universally. There will be some back-n-forth between > host and guest, but perhaps it will work okay. Thank you for the suggestion. > > >>> This is > >>> something I'm quite interested in for "virtgpu native contexts" (ie. > >>> native guest driver with new context type sitting on top of virtgpu), > >> > >> In a case of "native contexts" it should be doable, at least I can't see > >> any obvious problems. The madvise invocations could be passed to the > >> host using a new virtio-gpu command by the guest's madvise IOCTL > >> handler, instead-of/in-addition-to handling madvise in the guest's > >> kernel, and that's it. > > > > I think we don't want to do that, because MADV:WILLNEED would be by > > far the most frequent guest<->host synchronous round trip. So from > > that perspective tracking madvise state in guest kernel seems quite > > attractive. > > This is a valid concern. I'd assume that the overhead should be > tolerable, but I don't have any actual perf numbers. jfwiw, MADV:WILLNEED is a *very* hot path for gl drivers, based on some measurements I did a while back with various apps/benchmarks.. easily more than 10x the next most frequent ioctl (for MADV:WONTNEED and MADV:WILLNEED each, so more than 20x combined.. but MADV:WONTNEED can be async). But if the balloon triggering shrinker approach works out, that would be pretty great.. it seems like the easy option and doesn't require adding new host kernel uabi :-) BR, -R > > If we really can't track madvise state in the guest for dealing with > > host memory pressure, I think the better option is to introduce > > MADV:WILLNEED_REPLACE, ie. something to tell the host kernel that the > > buffer is needed but the previous contents are not (as long as the GPU > > VA remains the same). With this the host could allocate new pages if > > needed, and the guest would not need to wait for a reply from host. > > If variant with the memory ballooning will work, then it will be > possible to track the state within guest-only. Let's consider the > simplest variant for now. > > I'll try to implement the balloon driver support in the v2 and will get > back to you. > > >>> since that isn't using host storage > >> > >> s/host/guest ? > > > > Yes, sorry, I meant that it is not using guest storage. > > Thank you for the clarification. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v1 0/5] Add memory shrinker to VirtIO-GPU DRM driver
On Tue, Mar 8, 2022 at 11:28 AM Dmitry Osipenko wrote: > > On 3/8/22 19:29, Rob Clark wrote: > > On Tue, Mar 8, 2022 at 5:17 AM Dmitry Osipenko > > wrote: > >> > >> Hello, > >> > >> This patchset introduces memory shrinker for the VirtIO-GPU DRM driver. > >> During OOM, the shrinker will release BOs that are marked as "not needed" > >> by userspace using the new madvise IOCTL. The userspace in this case is > >> the Mesa VirGL driver, it will mark the cached BOs as "not needed", > >> allowing kernel driver to release memory of the cached shmem BOs on lowmem > >> situations, preventing OOM kills. > > > > Will host memory pressure already trigger shrinker in guest? > > The host memory pressure won't trigger shrinker in guest here. This > series will help only with the memory pressure within the guest using a > usual "virgl context". > > Having a host shrinker in a case of "virgl contexts" should be a > difficult problem to solve. Hmm, I think we just need the balloon driver to trigger the shrinker in the guest kernel? I suppose a driver like drm/virtio might want to differentiate between host and guest pressure (ie. consider only objects that have host vs guest storage), but even without that, freeing up memory in the guest when host is under memory pressure seems worthwhile. Maybe I'm over-simplifying? > > This is > > something I'm quite interested in for "virtgpu native contexts" (ie. > > native guest driver with new context type sitting on top of virtgpu), > > In a case of "native contexts" it should be doable, at least I can't see > any obvious problems. The madvise invocations could be passed to the > host using a new virtio-gpu command by the guest's madvise IOCTL > handler, instead-of/in-addition-to handling madvise in the guest's > kernel, and that's it. I think we don't want to do that, because MADV:WILLNEED would be by far the most frequent guest<->host synchronous round trip. So from that perspective tracking madvise state in guest kernel seems quite attractive. If we really can't track madvise state in the guest for dealing with host memory pressure, I think the better option is to introduce MADV:WILLNEED_REPLACE, ie. something to tell the host kernel that the buffer is needed but the previous contents are not (as long as the GPU VA remains the same). With this the host could allocate new pages if needed, and the guest would not need to wait for a reply from host. > > since that isn't using host storage > > s/host/guest ? Yes, sorry, I meant that it is not using guest storage. BR, -R ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v1 0/5] Add memory shrinker to VirtIO-GPU DRM driver
On Tue, Mar 8, 2022 at 5:17 AM Dmitry Osipenko wrote: > > Hello, > > This patchset introduces memory shrinker for the VirtIO-GPU DRM driver. > During OOM, the shrinker will release BOs that are marked as "not needed" > by userspace using the new madvise IOCTL. The userspace in this case is > the Mesa VirGL driver, it will mark the cached BOs as "not needed", > allowing kernel driver to release memory of the cached shmem BOs on lowmem > situations, preventing OOM kills. Will host memory pressure already trigger shrinker in guest? This is something I'm quite interested in for "virtgpu native contexts" (ie. native guest driver with new context type sitting on top of virtgpu), since that isn't using host storage BR, -R > This patchset includes couple fixes for problems I found while was working > on the shrinker, it also includes prerequisite DMA API usage improvement > needed by the shrinker. > > The Mesa and IGT patches will be kept on hold until this kernel series > will be approved and applied. > > This patchset was tested using Qemu and crosvm, including both cases of > IOMMU off/on. > > Mesa: https://gitlab.freedesktop.org/digetx/mesa/-/commits/virgl-madvise > IGT: > https://gitlab.freedesktop.org/digetx/igt-gpu-tools/-/tree/virtio-madvise > > Dmitry Osipenko (5): > drm/virtio: Correct drm_gem_shmem_get_sg_table() error handling > drm/virtio: Check whether transferred 2D BO is shmem > drm/virtio: Unlock GEM reservations in error code path > drm/virtio: Improve DMA API usage for shmem BOs > drm/virtio: Add memory shrinker > > drivers/gpu/drm/virtio/Makefile | 3 +- > drivers/gpu/drm/virtio/virtgpu_drv.c | 22 +++- > drivers/gpu/drm/virtio/virtgpu_drv.h | 31 - > drivers/gpu/drm/virtio/virtgpu_gem.c | 84 > drivers/gpu/drm/virtio/virtgpu_gem_shrinker.c | 124 ++ > drivers/gpu/drm/virtio/virtgpu_ioctl.c| 37 ++ > drivers/gpu/drm/virtio/virtgpu_kms.c | 17 ++- > drivers/gpu/drm/virtio/virtgpu_object.c | 63 +++-- > drivers/gpu/drm/virtio/virtgpu_plane.c| 17 ++- > drivers/gpu/drm/virtio/virtgpu_vq.c | 30 +++-- > include/uapi/drm/virtgpu_drm.h| 14 ++ > 11 files changed, 373 insertions(+), 69 deletions(-) > create mode 100644 drivers/gpu/drm/virtio/virtgpu_gem_shrinker.c > > -- > 2.35.1 > ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] drm/virtio: Remove restriction of non-zero blob_flags
From: Rob Clark
With native userspace drivers in guest, a lot of GEM objects need to be
neither shared nor mappable. And in fact making everything mappable
and/or sharable results in unreasonably high fd usage in host VMM.
Signed-off-by: Rob Clark
---
This is for a thing I'm working on, a new virtgpu context type that
allows for running native userspace driver in the guest, with a
thin shim in the host VMM. In this case, the guest has a lot of
GEM buffer objects which need to be neither shared nor mappable.
This supersedes https://patchwork.freedesktop.org/patch/475127/
drivers/gpu/drm/virtio/virtgpu_ioctl.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index 69f1952f3144..3a8078f2ee27 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -617,8 +617,7 @@ static int verify_blob(struct virtio_gpu_device *vgdev,
if (!vgdev->has_resource_blob)
return -EINVAL;
- if ((rc_blob->blob_flags & ~VIRTGPU_BLOB_FLAG_USE_MASK) ||
- !rc_blob->blob_flags)
+ if (rc_blob->blob_flags & ~VIRTGPU_BLOB_FLAG_USE_MASK)
return -EINVAL;
if (rc_blob->blob_flags & VIRTGPU_BLOB_FLAG_USE_CROSS_DEVICE) {
--
2.34.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] drm/virtio: Add USE_INTERNAL blob flag
On Fri, Feb 18, 2022 at 8:42 AM Chia-I Wu wrote:
>
> On Fri, Feb 18, 2022 at 7:57 AM Rob Clark wrote:
> >
> > From: Rob Clark
> >
> > With native userspace drivers in guest, a lot of GEM objects need to be
> > neither shared nor mappable. And in fact making everything mappable
> > and/or sharable results in unreasonably high fd usage in host VMM.
> >
> > Signed-off-by: Rob Clark
> > ---
> > This is for a thing I'm working on, a new virtgpu context type that
> > allows for running native userspace driver in the guest, with a
> > thin shim in the host VMM. In this case, the guest has a lot of
> > GEM buffer objects which need to be neither shared nor mappable.
> >
> > Alternative idea is to just drop the restriction that blob_flags
> > be non-zero. I'm ok with either approach.
> Dropping the restriction sounds better to me.
>
> What is the use case for such a resource? Does the host need to know
> such a resource exists?
There are a bunch of use cases, some internal (like visibility stream
buffers filled during binning pass and consumed during draw pass),
some external (tiled and/or UBWC buffers are never accessed on the
CPU).
In theory, at least currently, drm/virtgpu does not need to know about
them, but there are a lot of places in userspace that expect to have a
gem handle. Longer term, I think I want to extend virtgpu with
MADVISE ioctl so we can track DONTNEED state in guest and only release
buffers when host and/or guest is under memory pressure. For that we
will defn need guest side gem handles
BR,
-R
> >
> > drivers/gpu/drm/virtio/virtgpu_ioctl.c | 7 ++-
> > include/uapi/drm/virtgpu_drm.h | 1 +
> > 2 files changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> > b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> > index 69f1952f3144..92e1ba6b8078 100644
> > --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> > +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> > @@ -36,7 +36,8 @@
> >
> > #define VIRTGPU_BLOB_FLAG_USE_MASK (VIRTGPU_BLOB_FLAG_USE_MAPPABLE | \
> > VIRTGPU_BLOB_FLAG_USE_SHAREABLE | \
> > - VIRTGPU_BLOB_FLAG_USE_CROSS_DEVICE)
> > + VIRTGPU_BLOB_FLAG_USE_CROSS_DEVICE | \
> > + VIRTGPU_BLOB_FLAG_USE_INTERNAL)
> >
> > static int virtio_gpu_fence_event_create(struct drm_device *dev,
> > struct drm_file *file,
> > @@ -662,6 +663,10 @@ static int verify_blob(struct virtio_gpu_device *vgdev,
> > params->size = rc_blob->size;
> > params->blob = true;
> > params->blob_flags = rc_blob->blob_flags;
> > +
> > + /* USE_INTERNAL is local to guest kernel, don't past to host: */
> > + params->blob_flags &= ~VIRTGPU_BLOB_FLAG_USE_INTERNAL;
> > +
> > return 0;
> > }
> >
> > diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
> > index 0512fde5e697..62b7483e5c60 100644
> > --- a/include/uapi/drm/virtgpu_drm.h
> > +++ b/include/uapi/drm/virtgpu_drm.h
> > @@ -163,6 +163,7 @@ struct drm_virtgpu_resource_create_blob {
> > #define VIRTGPU_BLOB_FLAG_USE_MAPPABLE 0x0001
> > #define VIRTGPU_BLOB_FLAG_USE_SHAREABLE0x0002
> > #define VIRTGPU_BLOB_FLAG_USE_CROSS_DEVICE 0x0004
> > +#define VIRTGPU_BLOB_FLAG_USE_INTERNAL 0x0008 /* not-mappable,
> > not-shareable */
> > /* zero is invalid blob_mem */
> > __u32 blob_mem;
> > __u32 blob_flags;
> > --
> > 2.34.1
> >
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] drm/virtio: Add USE_INTERNAL blob flag
From: Rob Clark
With native userspace drivers in guest, a lot of GEM objects need to be
neither shared nor mappable. And in fact making everything mappable
and/or sharable results in unreasonably high fd usage in host VMM.
Signed-off-by: Rob Clark
---
This is for a thing I'm working on, a new virtgpu context type that
allows for running native userspace driver in the guest, with a
thin shim in the host VMM. In this case, the guest has a lot of
GEM buffer objects which need to be neither shared nor mappable.
Alternative idea is to just drop the restriction that blob_flags
be non-zero. I'm ok with either approach.
drivers/gpu/drm/virtio/virtgpu_ioctl.c | 7 ++-
include/uapi/drm/virtgpu_drm.h | 1 +
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index 69f1952f3144..92e1ba6b8078 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -36,7 +36,8 @@
#define VIRTGPU_BLOB_FLAG_USE_MASK (VIRTGPU_BLOB_FLAG_USE_MAPPABLE | \
VIRTGPU_BLOB_FLAG_USE_SHAREABLE | \
- VIRTGPU_BLOB_FLAG_USE_CROSS_DEVICE)
+ VIRTGPU_BLOB_FLAG_USE_CROSS_DEVICE | \
+ VIRTGPU_BLOB_FLAG_USE_INTERNAL)
static int virtio_gpu_fence_event_create(struct drm_device *dev,
struct drm_file *file,
@@ -662,6 +663,10 @@ static int verify_blob(struct virtio_gpu_device *vgdev,
params->size = rc_blob->size;
params->blob = true;
params->blob_flags = rc_blob->blob_flags;
+
+ /* USE_INTERNAL is local to guest kernel, don't past to host: */
+ params->blob_flags &= ~VIRTGPU_BLOB_FLAG_USE_INTERNAL;
+
return 0;
}
diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
index 0512fde5e697..62b7483e5c60 100644
--- a/include/uapi/drm/virtgpu_drm.h
+++ b/include/uapi/drm/virtgpu_drm.h
@@ -163,6 +163,7 @@ struct drm_virtgpu_resource_create_blob {
#define VIRTGPU_BLOB_FLAG_USE_MAPPABLE 0x0001
#define VIRTGPU_BLOB_FLAG_USE_SHAREABLE0x0002
#define VIRTGPU_BLOB_FLAG_USE_CROSS_DEVICE 0x0004
+#define VIRTGPU_BLOB_FLAG_USE_INTERNAL 0x0008 /* not-mappable,
not-shareable */
/* zero is invalid blob_mem */
__u32 blob_mem;
__u32 blob_flags;
--
2.34.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] drm/virtio: Fix capset-id query size
From: Rob Clark
The UABI was already defined for pointer to 64b value, and all the
userspace users of this ioctl that I could find are already using a
uint64_t (but zeroing it out to work around kernel only copying 32b).
Unfortunately this ioctl doesn't have a length field, so out of paranoia
I restricted the change to copy 64b to the single 64b param that can be
queried.
Fixes: 78aa20fa4381 ("drm/virtio: implement context init: advertise feature to
userspace")
Signed-off-by: Rob Clark
---
drivers/gpu/drm/virtio/virtgpu_ioctl.c | 16
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index 0f2f3f54dbf9..0158d27d5645 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -269,7 +269,8 @@ static int virtio_gpu_getparam_ioctl(struct drm_device
*dev, void *data,
{
struct virtio_gpu_device *vgdev = dev->dev_private;
struct drm_virtgpu_getparam *param = data;
- int value;
+ int value, ret, sz = sizeof(int);
+ uint64_t value64;
switch (param->param) {
case VIRTGPU_PARAM_3D_FEATURES:
@@ -291,13 +292,20 @@ static int virtio_gpu_getparam_ioctl(struct drm_device
*dev, void *data,
value = vgdev->has_context_init ? 1 : 0;
break;
case VIRTGPU_PARAM_SUPPORTED_CAPSET_IDs:
- value = vgdev->capset_id_mask;
+ value64 = vgdev->capset_id_mask;
+ sz = sizeof(value64);
break;
default:
return -EINVAL;
}
- if (copy_to_user(u64_to_user_ptr(param->value), &value, sizeof(int)))
- return -EFAULT;
+
+ if (sz == sizeof(int)) {
+ if (copy_to_user(u64_to_user_ptr(param->value), &value, sz))
+ return -EFAULT;
+ } else {
+ if (copy_to_user(u64_to_user_ptr(param->value), &value64, sz))
+ return -EFAULT;
+ }
return 0;
}
--
2.34.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Freedreno] [PATCH 16/17] iommu: remove DOMAIN_ATTR_IO_PGTABLE_CFG
On Thu, Mar 4, 2021 at 7:48 AM Robin Murphy wrote:
>
> On 2021-03-01 08:42, Christoph Hellwig wrote:
> > Signed-off-by: Christoph Hellwig
>
> Moreso than the previous patch, where the feature is at least relatively
> generic (note that there's a bunch of in-flight development around
> DOMAIN_ATTR_NESTING), I'm really not convinced that it's beneficial to
> bloat the generic iommu_ops structure with private driver-specific
> interfaces. The attribute interface is a great compromise for these
> kinds of things, and you can easily add type-checked wrappers around it
> for external callers (maybe even make the actual attributes internal
> between the IOMMU core and drivers) if that's your concern.
I suppose if this is *just* for the GPU we could move it into adreno_smmu_priv..
But one thing I'm not sure about is whether
IO_PGTABLE_QUIRK_ARM_OUTER_WBWA is something that other devices
*should* be using as well, but just haven't gotten around to yet.
BR,
-R
> Robin.
>
> > ---
> > drivers/gpu/drm/msm/adreno/adreno_gpu.c | 2 +-
> > drivers/iommu/arm/arm-smmu/arm-smmu.c | 40 +++--
> > drivers/iommu/iommu.c | 9 ++
> > include/linux/iommu.h | 9 +-
> > 4 files changed, 29 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > index 0f184c3dd9d9ec..78d98ab2ee3a68 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > @@ -191,7 +191,7 @@ void adreno_set_llc_attributes(struct iommu_domain
> > *iommu)
> > struct io_pgtable_domain_attr pgtbl_cfg;
> >
> > pgtbl_cfg.quirks = IO_PGTABLE_QUIRK_ARM_OUTER_WBWA;
> > - iommu_domain_set_attr(iommu, DOMAIN_ATTR_IO_PGTABLE_CFG, &pgtbl_cfg);
> > + iommu_domain_set_pgtable_attr(iommu, &pgtbl_cfg);
> > }
> >
> > struct msm_gem_address_space *
> > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > index 2e17d990d04481..2858999c86dfd1 100644
> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > @@ -1515,40 +1515,22 @@ static int arm_smmu_domain_enable_nesting(struct
> > iommu_domain *domain)
> > return ret;
> > }
> >
> > -static int arm_smmu_domain_set_attr(struct iommu_domain *domain,
> > - enum iommu_attr attr, void *data)
> > +static int arm_smmu_domain_set_pgtable_attr(struct iommu_domain *domain,
> > + struct io_pgtable_domain_attr *pgtbl_cfg)
> > {
> > - int ret = 0;
> > struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> > + int ret = -EPERM;
> >
> > - mutex_lock(&smmu_domain->init_mutex);
> > -
> > - switch(domain->type) {
> > - case IOMMU_DOMAIN_UNMANAGED:
> > - switch (attr) {
> > - case DOMAIN_ATTR_IO_PGTABLE_CFG: {
> > - struct io_pgtable_domain_attr *pgtbl_cfg = data;
> > -
> > - if (smmu_domain->smmu) {
> > - ret = -EPERM;
> > - goto out_unlock;
> > - }
> > + if (domain->type != IOMMU_DOMAIN_UNMANAGED)
> > + return -EINVAL;
> >
> > - smmu_domain->pgtbl_cfg = *pgtbl_cfg;
> > - break;
> > - }
> > - default:
> > - ret = -ENODEV;
> > - }
> > - break;
> > - case IOMMU_DOMAIN_DMA:
> > - ret = -ENODEV;
> > - break;
> > - default:
> > - ret = -EINVAL;
> > + mutex_lock(&smmu_domain->init_mutex);
> > + if (!smmu_domain->smmu) {
> > + smmu_domain->pgtbl_cfg = *pgtbl_cfg;
> > + ret = 0;
> > }
> > -out_unlock:
> > mutex_unlock(&smmu_domain->init_mutex);
> > +
> > return ret;
> > }
> >
> > @@ -1609,7 +1591,7 @@ static struct iommu_ops arm_smmu_ops = {
> > .device_group = arm_smmu_device_group,
> > .dma_use_flush_queue= arm_smmu_dma_use_flush_queue,
> > .dma_enable_flush_queue = arm_smmu_dma_enable_flush_queue,
> > - .domain_set_attr= arm_smmu_domain_set_attr,
> > + .domain_set_pgtable_attr = arm_smmu_domain_set_pgtable_attr,
> > .domain_enable_nesting = arm_smmu_domain_enable_nesting,
> > .of_xlate = arm_smmu_of_xlate,
> > .get_resv_regions = arm_smmu_get_resv_regions,
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 2e9e058501a953..8490aefd4b41f8 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -2693,6 +2693,15 @@ int iommu_domain_enable_nesting(struct iommu_domain
> > *domain)
> > }
> > EXPORT_SYMBOL_GPL(iommu_domain_enable_nesting);
> >
> > +int iommu_domain_set_pgtable_attr(struct iommu_domain *domain,
> > + struct io_pgtable_domain_attr *pgtbl_
[PATCH v3 2/3] drm: plumb attaching dev thru to prime_pin/unpin
From: Rob Clark
Needed in the following patch for cache operations.
Signed-off-by: Rob Clark
---
v3: rebased on drm-tip
drivers/gpu/drm/drm_gem.c | 8
drivers/gpu/drm/drm_internal.h | 4 ++--
drivers/gpu/drm/drm_prime.c | 4 ++--
drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 4 ++--
drivers/gpu/drm/msm/msm_drv.h | 4 ++--
drivers/gpu/drm/msm/msm_gem_prime.c | 4 ++--
drivers/gpu/drm/nouveau/nouveau_gem.h | 4 ++--
drivers/gpu/drm/nouveau/nouveau_prime.c | 4 ++--
drivers/gpu/drm/qxl/qxl_prime.c | 4 ++--
drivers/gpu/drm/radeon/radeon_prime.c | 4 ++--
drivers/gpu/drm/vgem/vgem_drv.c | 4 ++--
include/drm/drm_drv.h | 5 ++---
12 files changed, 26 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 84689ccae885..af2549c45027 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1215,22 +1215,22 @@ void drm_gem_print_info(struct drm_printer *p, unsigned
int indent,
obj->dev->driver->gem_print_info(p, indent, obj);
}
-int drm_gem_pin(struct drm_gem_object *obj)
+int drm_gem_pin(struct drm_gem_object *obj, struct device *dev)
{
if (obj->funcs && obj->funcs->pin)
return obj->funcs->pin(obj);
else if (obj->dev->driver->gem_prime_pin)
- return obj->dev->driver->gem_prime_pin(obj);
+ return obj->dev->driver->gem_prime_pin(obj, dev);
else
return 0;
}
-void drm_gem_unpin(struct drm_gem_object *obj)
+void drm_gem_unpin(struct drm_gem_object *obj, struct device *dev)
{
if (obj->funcs && obj->funcs->unpin)
obj->funcs->unpin(obj);
else if (obj->dev->driver->gem_prime_unpin)
- obj->dev->driver->gem_prime_unpin(obj);
+ obj->dev->driver->gem_prime_unpin(obj, dev);
}
void *drm_gem_vmap(struct drm_gem_object *obj)
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 51a2055c8f18..e64090373e3a 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -133,8 +133,8 @@ void drm_gem_release(struct drm_device *dev, struct
drm_file *file_private);
void drm_gem_print_info(struct drm_printer *p, unsigned int indent,
const struct drm_gem_object *obj);
-int drm_gem_pin(struct drm_gem_object *obj);
-void drm_gem_unpin(struct drm_gem_object *obj);
+int drm_gem_pin(struct drm_gem_object *obj, struct device *dev);
+void drm_gem_unpin(struct drm_gem_object *obj, struct device *dev);
void *drm_gem_vmap(struct drm_gem_object *obj);
void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr);
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 189d980402ad..126860432ff9 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -575,7 +575,7 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
{
struct drm_gem_object *obj = dma_buf->priv;
- return drm_gem_pin(obj);
+ return drm_gem_pin(obj, attach->dev);
}
EXPORT_SYMBOL(drm_gem_map_attach);
@@ -593,7 +593,7 @@ void drm_gem_map_detach(struct dma_buf *dma_buf,
{
struct drm_gem_object *obj = dma_buf->priv;
- drm_gem_unpin(obj);
+ drm_gem_unpin(obj, attach->dev);
}
EXPORT_SYMBOL(drm_gem_map_detach);
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
index a05292e8ed6f..67e69a5f00f2 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
@@ -43,7 +43,7 @@ int etnaviv_gem_prime_mmap(struct drm_gem_object *obj,
return etnaviv_obj->ops->mmap(etnaviv_obj, vma);
}
-int etnaviv_gem_prime_pin(struct drm_gem_object *obj)
+int etnaviv_gem_prime_pin(struct drm_gem_object *obj, struct device *dev)
{
if (!obj->import_attach) {
struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj);
@@ -55,7 +55,7 @@ int etnaviv_gem_prime_pin(struct drm_gem_object *obj)
return 0;
}
-void etnaviv_gem_prime_unpin(struct drm_gem_object *obj)
+void etnaviv_gem_prime_unpin(struct drm_gem_object *obj, struct device *dev)
{
if (!obj->import_attach) {
struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj);
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index ee7b512dc158..0eea68618b68 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -288,8 +288,8 @@ void msm_gem_prime_vunmap(struct drm_gem_object *obj, void
*vaddr);
int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma);
struct drm_gem_object *msm_gem_prime_import_sg_table(struct drm_device *dev,
s
[PATCH v2 2/3] drm: plumb attaching dev thru to prime_pin/unpin
From: Rob Clark
Needed in the following patch for cache operations.
Signed-off-by: Rob Clark
---
drivers/gpu/drm/drm_gem.c | 10 ++
drivers/gpu/drm/drm_gem_vram_helper.c | 6 --
drivers/gpu/drm/drm_prime.c | 4 ++--
drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 4 ++--
drivers/gpu/drm/msm/msm_drv.h | 4 ++--
drivers/gpu/drm/msm/msm_gem_prime.c | 4 ++--
drivers/gpu/drm/nouveau/nouveau_gem.h | 4 ++--
drivers/gpu/drm/nouveau/nouveau_prime.c | 4 ++--
drivers/gpu/drm/qxl/qxl_prime.c | 4 ++--
drivers/gpu/drm/radeon/radeon_prime.c | 4 ++--
drivers/gpu/drm/vboxvideo/vbox_prime.c | 4 ++--
drivers/gpu/drm/vgem/vgem_drv.c | 4 ++--
include/drm/drm_drv.h | 4 ++--
include/drm/drm_gem.h | 4 ++--
include/drm/drm_gem_vram_helper.h | 4 ++--
15 files changed, 36 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 7d6242cc69f2..0a2645769624 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1219,18 +1219,19 @@ void drm_gem_print_info(struct drm_printer *p, unsigned
int indent,
/**
* drm_gem_pin - Pin backing buffer in memory
* @obj: GEM object
+ * @dev: the device the buffer is being pinned for
*
* Make sure the backing buffer is pinned in memory.
*
* Returns:
* 0 on success or a negative error code on failure.
*/
-int drm_gem_pin(struct drm_gem_object *obj)
+int drm_gem_pin(struct drm_gem_object *obj, struct device *dev)
{
if (obj->funcs && obj->funcs->pin)
return obj->funcs->pin(obj);
else if (obj->dev->driver->gem_prime_pin)
- return obj->dev->driver->gem_prime_pin(obj);
+ return obj->dev->driver->gem_prime_pin(obj, dev);
else
return 0;
}
@@ -1239,15 +1240,16 @@ EXPORT_SYMBOL(drm_gem_pin);
/**
* drm_gem_unpin - Unpin backing buffer from memory
* @obj: GEM object
+ * @dev: the device the buffer is being pinned for
*
* Relax the requirement that the backing buffer is pinned in memory.
*/
-void drm_gem_unpin(struct drm_gem_object *obj)
+void drm_gem_unpin(struct drm_gem_object *obj, struct device *dev)
{
if (obj->funcs && obj->funcs->unpin)
obj->funcs->unpin(obj);
else if (obj->dev->driver->gem_prime_unpin)
- obj->dev->driver->gem_prime_unpin(obj);
+ obj->dev->driver->gem_prime_unpin(obj, dev);
}
EXPORT_SYMBOL(drm_gem_unpin);
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c
b/drivers/gpu/drm/drm_gem_vram_helper.c
index 4de782ca26b2..62fafec93948 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -548,7 +548,8 @@ EXPORT_SYMBOL(drm_gem_vram_driver_dumb_mmap_offset);
* 0 on success, or
* a negative errno code otherwise.
*/
-int drm_gem_vram_driver_gem_prime_pin(struct drm_gem_object *gem)
+int drm_gem_vram_driver_gem_prime_pin(struct drm_gem_object *gem,
+ struct device *dev)
{
struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem);
@@ -569,7 +570,8 @@ EXPORT_SYMBOL(drm_gem_vram_driver_gem_prime_pin);
Implements &struct drm_driver.gem_prime_unpin
* @gem: The GEM object to unpin
*/
-void drm_gem_vram_driver_gem_prime_unpin(struct drm_gem_object *gem)
+void drm_gem_vram_driver_gem_prime_unpin(struct drm_gem_object *gem,
+struct device *dev)
{
struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem);
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index d0c01318076b..505893cfac8e 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -196,7 +196,7 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
{
struct drm_gem_object *obj = dma_buf->priv;
- return drm_gem_pin(obj);
+ return drm_gem_pin(obj, attach->dev);
}
EXPORT_SYMBOL(drm_gem_map_attach);
@@ -213,7 +213,7 @@ void drm_gem_map_detach(struct dma_buf *dma_buf,
{
struct drm_gem_object *obj = dma_buf->priv;
- drm_gem_unpin(obj);
+ drm_gem_unpin(obj, attach->dev);
}
EXPORT_SYMBOL(drm_gem_map_detach);
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
index 00e8b6a817e3..44385d590aa7 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
@@ -43,7 +43,7 @@ int etnaviv_gem_prime_mmap(struct drm_gem_object *obj,
return etnaviv_obj->ops->mmap(etnaviv_obj, vma);
}
-int etnaviv_gem_prime_pin(struct drm_gem_object *obj)
+int etnaviv_gem_prime_pin(struct drm_gem_object *obj, struct device *dev)
{
if (!obj->import_attac
