From 7e78781df491e4beb475bac22e6c44236a5002d7 Mon Sep 17 00:00:00 2001 From: Gurchetan Singh Date: Mon, 22 Nov 2021 15:22:09 -0800 Subject: drm/virtgpu api: define a dummy fence signaled event The current virtgpu implementation of poll(..) drops events when VIRTGPU_CONTEXT_PARAM_POLL_RINGS_MASK is enabled (otherwise it's like a normal DRM driver). This is because paravirtualized userspaces receives responses in a buffer of type BLOB_MEM_GUEST, not by read(..). To be in line with other DRM drivers and avoid specialized behavior, it is possible to define a dummy event for virtgpu. Paravirtualized userspace will now have to call read(..) on the DRM fd to receive the dummy event. Fixes: b10790434cf2 ("drm/virtgpu api: create context init feature") Reported-by: Daniel Vetter Signed-off-by: Gurchetan Singh Reviewed-by: Daniel Vetter Link: http://patchwork.freedesktop.org/patch/msgid/20211122232210.602-2-gurchetansingh@google.com Signed-off-by: Gerd Hoffmann --- drivers/gpu/drm/virtio/virtgpu_drv.h | 1 - drivers/gpu/drm/virtio/virtgpu_ioctl.c | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) (limited to 'drivers/gpu') diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h index e0265fe74aa5..0a194aaad419 100644 --- a/drivers/gpu/drm/virtio/virtgpu_drv.h +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h @@ -138,7 +138,6 @@ struct virtio_gpu_fence_driver { spinlock_t lock; }; -#define VIRTGPU_EVENT_FENCE_SIGNALED_INTERNAL 0x10000000 struct virtio_gpu_fence_event { struct drm_pending_event base; struct drm_event event; diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c index 5618a1d5879c..3607646d3229 100644 --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c @@ -54,7 +54,7 @@ static int virtio_gpu_fence_event_create(struct drm_device *dev, if (!e) return -ENOMEM; - e->event.type = VIRTGPU_EVENT_FENCE_SIGNALED_INTERNAL; + e->event.type = VIRTGPU_EVENT_FENCE_SIGNALED; e->event.length = sizeof(e->event); ret = drm_event_reserve_init(dev, file, &e->base, &e->event); -- cgit v1.2.3 From 42abd0043e0c64fa64e99adba534c76b9b15e6b8 Mon Sep 17 00:00:00 2001 From: Gurchetan Singh Date: Mon, 22 Nov 2021 15:22:10 -0800 Subject: drm/virtio: use drm_poll(..) instead of virtio_gpu_poll(..) With the use of dummy events, we can drop virtgpu specific behavior. Fixes: cd7f5ca33585 ("drm/virtio: implement context init: add virtio_gpu_fence_event") Reported-by: Daniel Vetter Signed-off-by: Gurchetan Singh Reviewed-by: Daniel Vetter Link: http://patchwork.freedesktop.org/patch/msgid/20211122232210.602-3-gurchetansingh@google.com Signed-off-by: Gerd Hoffmann --- drivers/gpu/drm/virtio/virtgpu_drv.c | 42 +----------------------------------- 1 file changed, 1 insertion(+), 41 deletions(-) (limited to 'drivers/gpu') diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c index d86e1ad4a972..5072dbb0669a 100644 --- a/drivers/gpu/drm/virtio/virtgpu_drv.c +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c @@ -157,36 +157,6 @@ static void virtio_gpu_config_changed(struct virtio_device *vdev) schedule_work(&vgdev->config_changed_work); } -static __poll_t virtio_gpu_poll(struct file *filp, - struct poll_table_struct *wait) -{ - struct drm_file *drm_file = filp->private_data; - struct virtio_gpu_fpriv *vfpriv = drm_file->driver_priv; - struct drm_device *dev = drm_file->minor->dev; - struct virtio_gpu_device *vgdev = dev->dev_private; - struct drm_pending_event *e = NULL; - __poll_t mask = 0; - - if (!vgdev->has_virgl_3d || !vfpriv || !vfpriv->ring_idx_mask) - return drm_poll(filp, wait); - - poll_wait(filp, &drm_file->event_wait, wait); - - if (!list_empty(&drm_file->event_list)) { - spin_lock_irq(&dev->event_lock); - e = list_first_entry(&drm_file->event_list, - struct drm_pending_event, link); - drm_file->event_space += e->event->length; - list_del(&e->link); - spin_unlock_irq(&dev->event_lock); - - kfree(e); - mask |= EPOLLIN | EPOLLRDNORM; - } - - return mask; -} - static struct virtio_device_id id_table[] = { { VIRTIO_ID_GPU, VIRTIO_DEV_ANY_ID }, { 0 }, @@ -226,17 +196,7 @@ MODULE_AUTHOR("Dave Airlie "); MODULE_AUTHOR("Gerd Hoffmann "); MODULE_AUTHOR("Alon Levy"); -static const struct file_operations virtio_gpu_driver_fops = { - .owner = THIS_MODULE, - .open = drm_open, - .release = drm_release, - .unlocked_ioctl = drm_ioctl, - .compat_ioctl = drm_compat_ioctl, - .poll = virtio_gpu_poll, - .read = drm_read, - .llseek = noop_llseek, - .mmap = drm_gem_mmap -}; +DEFINE_DRM_GEM_FOPS(virtio_gpu_driver_fops); static const struct drm_driver driver = { .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_RENDER | DRIVER_ATOMIC, -- cgit v1.2.3 From 0c980a006d3fbee86c4d0698f66d6f5381831787 Mon Sep 17 00:00:00 2001 From: Maxime Ripard Date: Wed, 17 Nov 2021 10:45:22 +0100 Subject: drm/vc4: kms: Wait for the commit before increasing our clock rate Several DRM/KMS atomic commits can run in parallel if they affect different CRTC. These commits share the global HVS state, so we have some code to make sure we run commits in sequence. This synchronization code is one of the first thing that runs in vc4_atomic_commit_tail(). Another constraints we have is that we need to make sure the HVS clock gets a boost during the commit. That code relies on clk_set_min_rate and will remove the old minimum and set a new one. We also need another, temporary, minimum for the duration of the commit. The algorithm is thus to set a temporary minimum, drop the previous one, do the commit, and finally set the minimum for the current mode. However, the part that sets the temporary minimum and drops the older one runs before the commit synchronization code. Thus, under the proper conditions, we can end up mixing up the minimums and ending up with the wrong one for our current step. To avoid it, let's move the clock setup in the protected section. Fixes: d7d96c00e585 ("drm/vc4: hvs: Boost the core clock during modeset") Signed-off-by: Maxime Ripard Reviewed-by: Dave Stevenson Tested-by: Jian-Hong Pan Link: https://lore.kernel.org/r/20211117094527.146275-2-maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_kms.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'drivers/gpu') diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index f0b3e4cf5bce..764ddb41a4ce 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -353,9 +353,6 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state) vc4_hvs_mask_underrun(dev, vc4_crtc_state->assigned_channel); } - if (vc4->hvs->hvs5) - clk_set_min_rate(hvs->core_clk, 500000000); - old_hvs_state = vc4_hvs_get_old_global_state(state); if (!old_hvs_state) return; @@ -377,6 +374,9 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state) drm_err(dev, "Timed out waiting for commit\n"); } + if (vc4->hvs->hvs5) + clk_set_min_rate(hvs->core_clk, 500000000); + drm_atomic_helper_commit_modeset_disables(dev, state); vc4_ctm_commit(vc4, state); -- cgit v1.2.3 From f927767978d201d4ac023fcd797adbb963a6565d Mon Sep 17 00:00:00 2001 From: Maxime Ripard Date: Wed, 17 Nov 2021 10:45:23 +0100 Subject: drm/vc4: kms: Fix return code check The HVS global state functions return an error pointer, but in most cases we check if it's NULL, possibly resulting in an invalid pointer dereference. Fixes: 9ec03d7f1ed3 ("drm/vc4: kms: Wait on previous FIFO users before a commit") Signed-off-by: Maxime Ripard Reviewed-by: Dave Stevenson Tested-by: Jian-Hong Pan Link: https://lore.kernel.org/r/20211117094527.146275-3-maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_kms.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'drivers/gpu') diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index 764ddb41a4ce..3f780c195749 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -354,7 +354,7 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state) } old_hvs_state = vc4_hvs_get_old_global_state(state); - if (!old_hvs_state) + if (IS_ERR(old_hvs_state)) return; for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) { @@ -410,8 +410,8 @@ static int vc4_atomic_commit_setup(struct drm_atomic_state *state) unsigned int i; hvs_state = vc4_hvs_get_new_global_state(state); - if (!hvs_state) - return -EINVAL; + if (WARN_ON(IS_ERR(hvs_state))) + return PTR_ERR(hvs_state); for_each_new_crtc_in_state(state, crtc, crtc_state, i) { struct vc4_crtc_state *vc4_crtc_state = @@ -762,8 +762,8 @@ static int vc4_pv_muxing_atomic_check(struct drm_device *dev, unsigned int i; hvs_new_state = vc4_hvs_get_global_state(state); - if (!hvs_new_state) - return -EINVAL; + if (IS_ERR(hvs_new_state)) + return PTR_ERR(hvs_new_state); for (i = 0; i < ARRAY_SIZE(hvs_new_state->fifo_state); i++) if (!hvs_new_state->fifo_state[i].in_use) -- cgit v1.2.3 From 049cfff8d53a30cae3349ff71a4c01b7d9981bc2 Mon Sep 17 00:00:00 2001 From: Maxime Ripard Date: Wed, 17 Nov 2021 10:45:24 +0100 Subject: drm/vc4: kms: Add missing drm_crtc_commit_put Commit 9ec03d7f1ed3 ("drm/vc4: kms: Wait on previous FIFO users before a commit") introduced a global state for the HVS, with each FIFO storing the current CRTC commit so that we can properly synchronize commits. However, the refcounting was off and we thus ended up leaking the drm_crtc_commit structure every commit. Add a drm_crtc_commit_put to prevent the leakage. Fixes: 9ec03d7f1ed3 ("drm/vc4: kms: Wait on previous FIFO users before a commit") Signed-off-by: Maxime Ripard Reviewed-by: Dave Stevenson Tested-by: Jian-Hong Pan Link: https://lore.kernel.org/r/20211117094527.146275-4-maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_kms.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) (limited to 'drivers/gpu') diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index 3f780c195749..7c1d0c3beba2 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -361,6 +361,7 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state) struct vc4_crtc_state *vc4_crtc_state = to_vc4_crtc_state(old_crtc_state); unsigned int channel = vc4_crtc_state->assigned_channel; + struct drm_crtc_commit *commit; int ret; if (channel == VC4_HVS_CHANNEL_DISABLED) @@ -369,9 +370,15 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state) if (!old_hvs_state->fifo_state[channel].in_use) continue; - ret = drm_crtc_commit_wait(old_hvs_state->fifo_state[channel].pending_commit); + commit = old_hvs_state->fifo_state[channel].pending_commit; + if (!commit) + continue; + + ret = drm_crtc_commit_wait(commit); if (ret) drm_err(dev, "Timed out waiting for commit\n"); + + drm_crtc_commit_put(commit); } if (vc4->hvs->hvs5) -- cgit v1.2.3 From d134c5ff71c7f2320fc7997f2fbbdedf0c76889a Mon Sep 17 00:00:00 2001 From: Maxime Ripard Date: Wed, 17 Nov 2021 10:45:25 +0100 Subject: drm/vc4: kms: Clear the HVS FIFO commit pointer once done Commit 9ec03d7f1ed3 ("drm/vc4: kms: Wait on previous FIFO users before a commit") introduced a wait on the previous commit done on a given HVS FIFO. However, we never cleared that pointer once done. Since drm_crtc_commit_put can free the drm_crtc_commit structure directly if we were the last user, this means that it can lead to a use-after free if we were to duplicate the state, and that stale pointer would even be copied to the new state. Set the pointer to NULL once we're done with the wait so that we don't carry over a pointer to a free'd structure. Fixes: 9ec03d7f1ed3 ("drm/vc4: kms: Wait on previous FIFO users before a commit") Signed-off-by: Maxime Ripard Reviewed-by: Dave Stevenson Tested-by: Jian-Hong Pan Link: https://lore.kernel.org/r/20211117094527.146275-5-maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_kms.c | 1 + 1 file changed, 1 insertion(+) (limited to 'drivers/gpu') diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index 7c1d0c3beba2..f80370e87e98 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -379,6 +379,7 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state) drm_err(dev, "Timed out waiting for commit\n"); drm_crtc_commit_put(commit); + old_hvs_state->fifo_state[channel].pending_commit = NULL; } if (vc4->hvs->hvs5) -- cgit v1.2.3 From d354699e2292c60f25496d3c31ce4e7b1563b899 Mon Sep 17 00:00:00 2001 From: Maxime Ripard Date: Wed, 17 Nov 2021 10:45:26 +0100 Subject: drm/vc4: kms: Don't duplicate pending commit Our HVS global state, when duplicated, will also copy the pointer to the drm_crtc_commit (and increase the reference count) for each FIFO if the pointer is not NULL. However, our atomic_setup function will overwrite that pointer without putting the reference back leading to a memory leak. Since the commit is only relevant during the atomic commit process, it doesn't make sense to duplicate the reference to the commit anyway. Let's remove it. Fixes: 9ec03d7f1ed3 ("drm/vc4: kms: Wait on previous FIFO users before a commit") Signed-off-by: Maxime Ripard Reviewed-by: Dave Stevenson Tested-by: Jian-Hong Pan Link: https://lore.kernel.org/r/20211117094527.146275-6-maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_kms.c | 6 ------ 1 file changed, 6 deletions(-) (limited to 'drivers/gpu') diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index f80370e87e98..d9b3e3ad71ea 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -676,12 +676,6 @@ vc4_hvs_channels_duplicate_state(struct drm_private_obj *obj) for (i = 0; i < HVS_NUM_CHANNELS; i++) { state->fifo_state[i].in_use = old_state->fifo_state[i].in_use; - - if (!old_state->fifo_state[i].pending_commit) - continue; - - state->fifo_state[i].pending_commit = - drm_crtc_commit_get(old_state->fifo_state[i].pending_commit); } return &state->base; -- cgit v1.2.3 From 6052a3110be208e547a4a8aeb184446199a16e8a Mon Sep 17 00:00:00 2001 From: Maxime Ripard Date: Wed, 17 Nov 2021 10:45:27 +0100 Subject: drm/vc4: kms: Fix previous HVS commit wait Our current code is supposed to serialise the commits by waiting for all the drm_crtc_commits associated to the previous HVS state. However, assuming we have two CRTCs running and being configured and we configure each one alternately, we end up in a situation where we're not waiting at all. Indeed, starting with a state (state 0) where both CRTCs are running, and doing a commit (state 1) on the first CRTC (CRTC 0), we'll associate its commit to its assigned FIFO in vc4_hvs_state. If we get a new commit (state 2), this time affecting the second CRTC (CRTC 1), the DRM core will allow both commits to execute in parallel (assuming they don't have any share resources). Our code in vc4_atomic_commit_tail is supposed to make sure we only get one commit at a time and serialised by order of submission. It does so by using for_each_old_crtc_in_state, making sure that the CRTC has a FIFO assigned, is used, and has a commit pending. If it does, then we'll wait for the commit before going forward. During the transition from state 0 to state 1, as our old CRTC state we get the CRTC 0 state 0, its commit, we wait for it, everything works fine. During the transition from state 1 to state 2 though, the use of for_each_old_crtc_in_state is wrong. Indeed, while the code assumes it's returning the state of the CRTC in the old state (so CRTC 0 state 1), it actually returns the old state of the CRTC affected by the current commit, so CRTC 0 state 0 since it wasn't part of state 1. Due to this, if we alternate between the configuration of CRTC 0 and CRTC 1, we never actually wait for anything since we should be waiting on the other every time, but it never is affected by the previous commit. Change the logic to, at every commit, look at every FIFO in the previous HVS state, and if it's in use and has a commit associated to it, wait for that commit. Fixes: 9ec03d7f1ed3 ("drm/vc4: kms: Wait on previous FIFO users before a commit") Signed-off-by: Maxime Ripard Reviewed-by: Dave Stevenson Tested-by: Jian-Hong Pan Link: https://lore.kernel.org/r/20211117094527.146275-7-maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_kms.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) (limited to 'drivers/gpu') diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index d9b3e3ad71ea..b61792d2aa65 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -337,10 +337,10 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state) struct drm_device *dev = state->dev; struct vc4_dev *vc4 = to_vc4_dev(dev); struct vc4_hvs *hvs = vc4->hvs; - struct drm_crtc_state *old_crtc_state; struct drm_crtc_state *new_crtc_state; struct drm_crtc *crtc; struct vc4_hvs_state *old_hvs_state; + unsigned int channel; int i; for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { @@ -357,16 +357,10 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state) if (IS_ERR(old_hvs_state)) return; - for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) { - struct vc4_crtc_state *vc4_crtc_state = - to_vc4_crtc_state(old_crtc_state); - unsigned int channel = vc4_crtc_state->assigned_channel; + for (channel = 0; channel < HVS_NUM_CHANNELS; channel++) { struct drm_crtc_commit *commit; int ret; - if (channel == VC4_HVS_CHANNEL_DISABLED) - continue; - if (!old_hvs_state->fifo_state[channel].in_use) continue; -- cgit v1.2.3