* [PATCH 0/5] drm/vmwgfx: vblank and crc generation support
@ 2024-04-02 23:28 Zack Rusin
2024-04-02 23:28 ` [PATCH 1/5] drm/vmwgfx: Implement virtual kms Zack Rusin
` (4 more replies)
0 siblings, 5 replies; 16+ messages in thread
From: Zack Rusin @ 2024-04-02 23:28 UTC (permalink / raw)
To: dri-devel
Cc: Broadcom internal kernel review list, ian.forbes, martin.krastev,
maaz.mombasawala, Zack Rusin
vmwgfx didn't have support for vblank or crc generation which made it
impossible to use a large number of IGT tests to properly test DRM
functionality in the driver.
This series add virtual vblank and crc generation support, which allows
running most of IGT and immediately helped fix a number of kms issues
in the driver.
Zack Rusin (5):
drm/vmwgfx: Implement virtual kms
drm/vmwgfx: Implement virtual crc generation
drm/vmwgfx: Fix prime import/export
drm/vmwgfx: Fix crtc's atomic check conditional
drm/vmwgfx: Sort primary plane formats by order of preference
drivers/gpu/drm/vmwgfx/Makefile | 2 +-
drivers/gpu/drm/vmwgfx/vmwgfx_blit.c | 35 +-
drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 7 +-
drivers/gpu/drm/vmwgfx/vmwgfx_bo.h | 2 +
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 5 +
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 7 +
drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 32 ++
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 51 +-
drivers/gpu/drm/vmwgfx/vmwgfx_kms.h | 26 +-
drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 39 +-
drivers/gpu/drm/vmwgfx/vmwgfx_prime.c | 15 +-
drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 32 +-
drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 28 +-
drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 42 +-
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 44 +-
drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c | 630 +++++++++++++++++++++
drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h | 75 +++
17 files changed, 963 insertions(+), 109 deletions(-)
create mode 100644 drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
create mode 100644 drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h
--
2.40.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/5] drm/vmwgfx: Implement virtual kms
2024-04-02 23:28 [PATCH 0/5] drm/vmwgfx: vblank and crc generation support Zack Rusin
@ 2024-04-02 23:28 ` Zack Rusin
2024-04-05 21:53 ` Maaz Mombasawala
2024-04-02 23:28 ` [PATCH 2/5] drm/vmwgfx: Implement virtual crc generation Zack Rusin
` (3 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Zack Rusin @ 2024-04-02 23:28 UTC (permalink / raw)
To: dri-devel
Cc: Broadcom internal kernel review list, ian.forbes, martin.krastev,
maaz.mombasawala, Zack Rusin
By default vmwgfx doesn't support vblanking or crc generation which
makes it impossible to use various IGT tests to validate vmwgfx.
Implement virtual kernel mode setting, which is mainly related to
simulated vblank support.
Code is very similar to amd's vkms and the vkms module itself, except
that it's integrated with vmwgfx three different output technologies -
legacy, screen object and screen targets.
Make IGT's kms_vblank pass on vmwgfx and allows a lot of other IGT
tests to run with vmwgfx.
Support for vkms needs to be manually enabled by adding:
guestinfo.vmwgfx.vkms_enable = "TRUE"
somewhere in the vmx file, otherwise it's off by default.
Signed-off-by: Zack Rusin <zack.rusin@broadcom.com>
---
drivers/gpu/drm/vmwgfx/Makefile | 2 +-
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 3 +
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 2 +
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 15 ++-
drivers/gpu/drm/vmwgfx/vmwgfx_kms.h | 9 +-
drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 39 ++----
drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 28 ++--
drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 22 +--
drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c | 193 +++++++++++++++++++++++++++
drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h | 53 ++++++++
10 files changed, 302 insertions(+), 64 deletions(-)
create mode 100644 drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
create mode 100644 drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h
diff --git a/drivers/gpu/drm/vmwgfx/Makefile b/drivers/gpu/drm/vmwgfx/Makefile
index e94479d9cd5b..46a4ab688a7f 100644
--- a/drivers/gpu/drm/vmwgfx/Makefile
+++ b/drivers/gpu/drm/vmwgfx/Makefile
@@ -10,6 +10,6 @@ vmwgfx-y := vmwgfx_execbuf.o vmwgfx_gmr.o vmwgfx_kms.o vmwgfx_drv.o \
vmwgfx_simple_resource.o vmwgfx_va.o vmwgfx_blit.o \
vmwgfx_validation.o vmwgfx_page_dirty.o vmwgfx_streamoutput.o \
vmwgfx_devcaps.o ttm_object.o vmwgfx_system_manager.o \
- vmwgfx_gem.o
+ vmwgfx_gem.o vmwgfx_vkms.o
obj-$(CONFIG_DRM_VMWGFX) := vmwgfx.o
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index c7d90f96d16a..e34c48fd25d4 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -32,6 +32,7 @@
#include "vmwgfx_binding.h"
#include "vmwgfx_devcaps.h"
#include "vmwgfx_mksstat.h"
+#include "vmwgfx_vkms.h"
#include "ttm_object.h"
#include <drm/drm_aperture.h>
@@ -910,6 +911,8 @@ static int vmw_driver_load(struct vmw_private *dev_priv, u32 pci_id)
"Please switch to a supported graphics device to avoid problems.");
}
+ vmw_vkms_init(dev_priv);
+
ret = vmw_dma_select_mode(dev_priv);
if (unlikely(ret != 0)) {
drm_info(&dev_priv->drm,
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
index 01f41fbb9c3b..4f5d7d13c4aa 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
@@ -615,6 +615,8 @@ struct vmw_private {
uint32 *devcaps;
+ bool vkms_enabled;
+
/*
* mksGuestStat instance-descriptor and pid arrays
*/
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 09214f9339b2..e763cf0e6cfc 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -27,6 +27,7 @@
#include "vmwgfx_kms.h"
#include "vmwgfx_bo.h"
+#include "vmwgfx_vkms.h"
#include "vmw_surface_cache.h"
#include <drm/drm_atomic.h>
@@ -37,9 +38,16 @@
#include <drm/drm_sysfs.h>
#include <drm/drm_edid.h>
+void vmw_du_init(struct vmw_display_unit *du)
+{
+ hrtimer_init(&du->vkms.timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+ du->vkms.timer.function = &vmw_vkms_vblank_simulate;
+}
+
void vmw_du_cleanup(struct vmw_display_unit *du)
{
struct vmw_private *dev_priv = vmw_priv(du->primary.dev);
+ hrtimer_cancel(&du->vkms.timer);
drm_plane_cleanup(&du->primary);
if (vmw_cmd_supported(dev_priv))
drm_plane_cleanup(&du->cursor.base);
@@ -957,13 +965,6 @@ void vmw_du_crtc_atomic_begin(struct drm_crtc *crtc,
{
}
-
-void vmw_du_crtc_atomic_flush(struct drm_crtc *crtc,
- struct drm_atomic_state *state)
-{
-}
-
-
/**
* vmw_du_crtc_duplicate_state - duplicate crtc state
* @crtc: DRM crtc
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
index 4a2e3cac1c22..9e83a1553286 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
@@ -376,6 +376,12 @@ struct vmw_display_unit {
bool is_implicit;
int set_gui_x;
int set_gui_y;
+
+ struct {
+ struct hrtimer timer;
+ ktime_t period_ns;
+ struct drm_pending_vblank_event *event;
+ } vkms;
};
#define vmw_crtc_to_du(x) \
@@ -387,6 +393,7 @@ struct vmw_display_unit {
/*
* Shared display unit functions - vmwgfx_kms.c
*/
+void vmw_du_init(struct vmw_display_unit *du);
void vmw_du_cleanup(struct vmw_display_unit *du);
void vmw_du_crtc_save(struct drm_crtc *crtc);
void vmw_du_crtc_restore(struct drm_crtc *crtc);
@@ -473,8 +480,6 @@ int vmw_du_crtc_atomic_check(struct drm_crtc *crtc,
struct drm_atomic_state *state);
void vmw_du_crtc_atomic_begin(struct drm_crtc *crtc,
struct drm_atomic_state *state);
-void vmw_du_crtc_atomic_flush(struct drm_crtc *crtc,
- struct drm_atomic_state *state);
void vmw_du_crtc_reset(struct drm_crtc *crtc);
struct drm_crtc_state *vmw_du_crtc_duplicate_state(struct drm_crtc *crtc);
void vmw_du_crtc_destroy_state(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
index c4db4aecca6c..5befc2719a49 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
@@ -27,6 +27,7 @@
#include "vmwgfx_bo.h"
#include "vmwgfx_kms.h"
+#include "vmwgfx_vkms.h"
#include <drm/drm_atomic.h>
#include <drm/drm_atomic_helper.h>
@@ -241,33 +242,6 @@ static void vmw_ldu_crtc_mode_set_nofb(struct drm_crtc *crtc)
{
}
-/**
- * vmw_ldu_crtc_atomic_enable - Noop
- *
- * @crtc: CRTC associated with the new screen
- * @state: Unused
- *
- * This is called after a mode set has been completed. Here's
- * usually a good place to call vmw_ldu_add_active/vmw_ldu_del_active
- * but since for LDU the display plane is closely tied to the
- * CRTC, it makes more sense to do those at plane update time.
- */
-static void vmw_ldu_crtc_atomic_enable(struct drm_crtc *crtc,
- struct drm_atomic_state *state)
-{
-}
-
-/**
- * vmw_ldu_crtc_atomic_disable - Turns off CRTC
- *
- * @crtc: CRTC to be turned off
- * @state: Unused
- */
-static void vmw_ldu_crtc_atomic_disable(struct drm_crtc *crtc,
- struct drm_atomic_state *state)
-{
-}
-
static const struct drm_crtc_funcs vmw_legacy_crtc_funcs = {
.gamma_set = vmw_du_crtc_gamma_set,
.destroy = vmw_ldu_crtc_destroy,
@@ -276,6 +250,9 @@ static const struct drm_crtc_funcs vmw_legacy_crtc_funcs = {
.atomic_destroy_state = vmw_du_crtc_destroy_state,
.set_config = drm_atomic_helper_set_config,
.page_flip = drm_atomic_helper_page_flip,
+ .enable_vblank = vmw_vkms_enable_vblank,
+ .disable_vblank = vmw_vkms_disable_vblank,
+ .get_vblank_timestamp = vmw_vkms_get_vblank_timestamp,
};
@@ -418,9 +395,9 @@ static const struct drm_crtc_helper_funcs vmw_ldu_crtc_helper_funcs = {
.mode_set_nofb = vmw_ldu_crtc_mode_set_nofb,
.atomic_check = vmw_du_crtc_atomic_check,
.atomic_begin = vmw_du_crtc_atomic_begin,
- .atomic_flush = vmw_du_crtc_atomic_flush,
- .atomic_enable = vmw_ldu_crtc_atomic_enable,
- .atomic_disable = vmw_ldu_crtc_atomic_disable,
+ .atomic_flush = vmw_vkms_crtc_atomic_flush,
+ .atomic_enable = vmw_vkms_crtc_atomic_enable,
+ .atomic_disable = vmw_vkms_crtc_atomic_disable,
};
@@ -541,6 +518,8 @@ static int vmw_ldu_init(struct vmw_private *dev_priv, unsigned unit)
dev_priv->implicit_placement_property,
1);
+ vmw_du_init(&ldu->base);
+
return 0;
err_free_unregister:
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
index c6e646895f9e..df0039a8ef29 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
@@ -27,11 +27,13 @@
#include "vmwgfx_bo.h"
#include "vmwgfx_kms.h"
+#include "vmwgfx_vkms.h"
#include <drm/drm_atomic.h>
#include <drm/drm_atomic_helper.h>
#include <drm/drm_damage_helper.h>
#include <drm/drm_fourcc.h>
+#include <drm/drm_vblank.h>
#define vmw_crtc_to_sou(x) \
container_of(x, struct vmw_screen_object_unit, base.crtc)
@@ -267,19 +269,6 @@ static void vmw_sou_crtc_helper_prepare(struct drm_crtc *crtc)
{
}
-/**
- * vmw_sou_crtc_atomic_enable - Noop
- *
- * @crtc: CRTC associated with the new screen
- * @state: Unused
- *
- * This is called after a mode set has been completed.
- */
-static void vmw_sou_crtc_atomic_enable(struct drm_crtc *crtc,
- struct drm_atomic_state *state)
-{
-}
-
/**
* vmw_sou_crtc_atomic_disable - Turns off CRTC
*
@@ -302,6 +291,9 @@ static void vmw_sou_crtc_atomic_disable(struct drm_crtc *crtc,
sou = vmw_crtc_to_sou(crtc);
dev_priv = vmw_priv(crtc->dev);
+ if (dev_priv->vkms_enabled)
+ drm_crtc_vblank_off(crtc);
+
if (sou->defined) {
ret = vmw_sou_fifo_destroy(dev_priv, sou);
if (ret)
@@ -317,6 +309,9 @@ static const struct drm_crtc_funcs vmw_screen_object_crtc_funcs = {
.atomic_destroy_state = vmw_du_crtc_destroy_state,
.set_config = drm_atomic_helper_set_config,
.page_flip = drm_atomic_helper_page_flip,
+ .enable_vblank = vmw_vkms_enable_vblank,
+ .disable_vblank = vmw_vkms_disable_vblank,
+ .get_vblank_timestamp = vmw_vkms_get_vblank_timestamp,
};
/*
@@ -794,8 +789,8 @@ static const struct drm_crtc_helper_funcs vmw_sou_crtc_helper_funcs = {
.mode_set_nofb = vmw_sou_crtc_mode_set_nofb,
.atomic_check = vmw_du_crtc_atomic_check,
.atomic_begin = vmw_du_crtc_atomic_begin,
- .atomic_flush = vmw_du_crtc_atomic_flush,
- .atomic_enable = vmw_sou_crtc_atomic_enable,
+ .atomic_flush = vmw_vkms_crtc_atomic_flush,
+ .atomic_enable = vmw_vkms_crtc_atomic_enable,
.atomic_disable = vmw_sou_crtc_atomic_disable,
};
@@ -905,6 +900,9 @@ static int vmw_sou_init(struct vmw_private *dev_priv, unsigned unit)
dev->mode_config.suggested_x_property, 0);
drm_object_attach_property(&connector->base,
dev->mode_config.suggested_y_property, 0);
+
+ vmw_du_init(&sou->base);
+
return 0;
err_free_unregister:
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
index 3c8414a13dba..665bde7e0be0 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
@@ -27,12 +27,14 @@
#include "vmwgfx_bo.h"
#include "vmwgfx_kms.h"
+#include "vmwgfx_vkms.h"
#include "vmw_surface_cache.h"
#include <drm/drm_atomic.h>
#include <drm/drm_atomic_helper.h>
#include <drm/drm_damage_helper.h>
#include <drm/drm_fourcc.h>
+#include <drm/drm_vblank.h>
#define vmw_crtc_to_stdu(x) \
container_of(x, struct vmw_screen_target_display_unit, base.crtc)
@@ -412,11 +414,6 @@ static void vmw_stdu_crtc_helper_prepare(struct drm_crtc *crtc)
{
}
-static void vmw_stdu_crtc_atomic_enable(struct drm_crtc *crtc,
- struct drm_atomic_state *state)
-{
-}
-
static void vmw_stdu_crtc_atomic_disable(struct drm_crtc *crtc,
struct drm_atomic_state *state)
{
@@ -424,7 +421,6 @@ static void vmw_stdu_crtc_atomic_disable(struct drm_crtc *crtc,
struct vmw_screen_target_display_unit *stdu;
int ret;
-
if (!crtc) {
DRM_ERROR("CRTC is NULL\n");
return;
@@ -433,6 +429,9 @@ static void vmw_stdu_crtc_atomic_disable(struct drm_crtc *crtc,
stdu = vmw_crtc_to_stdu(crtc);
dev_priv = vmw_priv(crtc->dev);
+ if (dev_priv->vkms_enabled)
+ drm_crtc_vblank_off(crtc);
+
if (stdu->defined) {
ret = vmw_stdu_bind_st(dev_priv, stdu, NULL);
if (ret)
@@ -770,7 +769,6 @@ int vmw_kms_stdu_surface_dirty(struct vmw_private *dev_priv,
return ret;
}
-
/*
* Screen Target CRTC dispatch table
*/
@@ -782,6 +780,9 @@ static const struct drm_crtc_funcs vmw_stdu_crtc_funcs = {
.atomic_destroy_state = vmw_du_crtc_destroy_state,
.set_config = drm_atomic_helper_set_config,
.page_flip = drm_atomic_helper_page_flip,
+ .enable_vblank = vmw_vkms_enable_vblank,
+ .disable_vblank = vmw_vkms_disable_vblank,
+ .get_vblank_timestamp = vmw_vkms_get_vblank_timestamp,
};
@@ -1457,8 +1458,8 @@ static const struct drm_crtc_helper_funcs vmw_stdu_crtc_helper_funcs = {
.mode_set_nofb = vmw_stdu_crtc_mode_set_nofb,
.atomic_check = vmw_du_crtc_atomic_check,
.atomic_begin = vmw_du_crtc_atomic_begin,
- .atomic_flush = vmw_du_crtc_atomic_flush,
- .atomic_enable = vmw_stdu_crtc_atomic_enable,
+ .atomic_flush = vmw_vkms_crtc_atomic_flush,
+ .atomic_enable = vmw_vkms_crtc_atomic_enable,
.atomic_disable = vmw_stdu_crtc_atomic_disable,
};
@@ -1575,6 +1576,9 @@ static int vmw_stdu_init(struct vmw_private *dev_priv, unsigned unit)
dev->mode_config.suggested_x_property, 0);
drm_object_attach_property(&connector->base,
dev->mode_config.suggested_y_property, 0);
+
+ vmw_du_init(&stdu->base);
+
return 0;
err_free_unregister:
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
new file mode 100644
index 000000000000..ff76bfd70f91
--- /dev/null
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
@@ -0,0 +1,193 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+/**************************************************************************
+ *
+ * Copyright (c) 2024 Broadcom. All Rights Reserved. The term
+ * “Broadcom” refers to Broadcom Inc. and/or its subsidiaries.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sub license, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial portions
+ * of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
+ * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+ * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+ * USE OR OTHER DEALINGS IN THE SOFTWARE.
+ *
+ **************************************************************************/
+
+#include "vmwgfx_vkms.h"
+
+#include "vmwgfx_drv.h"
+#include "vmwgfx_kms.h"
+#include "vmwgfx_vkms.h"
+
+#include <drm/drm_crtc.h>
+#include <drm/drm_print.h>
+#include <drm/drm_vblank.h>
+
+#define GUESTINFO_VBLANK "guestinfo.vmwgfx.vkms_enable"
+
+enum hrtimer_restart
+vmw_vkms_vblank_simulate(struct hrtimer *timer)
+{
+ struct vmw_display_unit *du = container_of(timer, struct vmw_display_unit, vkms.timer);
+ struct drm_crtc *crtc = &du->crtc;
+ u64 ret_overrun;
+ bool ret;
+
+ ret_overrun = hrtimer_forward_now(&du->vkms.timer,
+ du->vkms.period_ns);
+ if (ret_overrun != 1)
+ DRM_WARN("%s: vblank timer overrun\n", __func__);
+
+ ret = drm_crtc_handle_vblank(crtc);
+ /* Don't queue timer again when vblank is disabled. */
+ if (!ret)
+ return HRTIMER_NORESTART;
+
+ return HRTIMER_RESTART;
+}
+
+void
+vmw_vkms_init(struct vmw_private *vmw)
+{
+ char buffer[64];
+ const size_t max_buf_len = sizeof(buffer) - 1;
+ size_t buf_len = max_buf_len;
+ int ret;
+
+ vmw->vkms_enabled = false;
+
+ ret = vmw_host_get_guestinfo(GUESTINFO_VBLANK, buffer, &buf_len);
+ if (ret || buf_len > max_buf_len)
+ return;
+ buffer[buf_len] = '\0';
+
+ ret = kstrtobool(buffer, &vmw->vkms_enabled);
+ if (!ret && vmw->vkms_enabled) {
+ ret = drm_vblank_init(&vmw->drm, VMWGFX_NUM_DISPLAY_UNITS);
+ vmw->vkms_enabled = (ret == 0);
+ drm_info(&vmw->drm, "vkms_enabled = %d\n", vmw->vkms_enabled);
+ }
+}
+
+bool
+vmw_vkms_get_vblank_timestamp(struct drm_crtc *crtc,
+ int *max_error,
+ ktime_t *vblank_time,
+ bool in_vblank_irq)
+{
+ struct drm_device *dev = crtc->dev;
+ struct vmw_private *vmw = vmw_priv(dev);
+ unsigned int pipe = crtc->index;
+ struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
+ struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
+
+ if (!vmw->vkms_enabled)
+ return false;
+
+ if (!READ_ONCE(vblank->enabled)) {
+ *vblank_time = ktime_get();
+ return true;
+ }
+
+ *vblank_time = READ_ONCE(du->vkms.timer.node.expires);
+
+ if (WARN_ON(*vblank_time == vblank->time))
+ return true;
+
+ /*
+ * To prevent races we roll the hrtimer forward before we do any
+ * interrupt processing - this is how real hw works (the interrupt is
+ * only generated after all the vblank registers are updated) and what
+ * the vblank core expects. Therefore we need to always correct the
+ * timestampe by one frame.
+ */
+ *vblank_time -= du->vkms.period_ns;
+
+ return true;
+}
+
+int
+vmw_vkms_enable_vblank(struct drm_crtc *crtc)
+{
+ struct drm_device *dev = crtc->dev;
+ struct vmw_private *vmw = vmw_priv(dev);
+ unsigned int pipe = drm_crtc_index(crtc);
+ struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
+ struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
+
+ if (!vmw->vkms_enabled)
+ return -EINVAL;
+
+ drm_calc_timestamping_constants(crtc, &crtc->mode);
+
+ du->vkms.period_ns = ktime_set(0, vblank->framedur_ns);
+ hrtimer_start(&du->vkms.timer, du->vkms.period_ns, HRTIMER_MODE_REL);
+
+ return 0;
+}
+
+void
+vmw_vkms_disable_vblank(struct drm_crtc *crtc)
+{
+ struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
+ struct vmw_private *vmw = vmw_priv(crtc->dev);
+
+ if (!vmw->vkms_enabled)
+ return;
+
+ hrtimer_try_to_cancel(&du->vkms.timer);
+}
+
+void
+vmw_vkms_crtc_atomic_flush(struct drm_crtc *crtc,
+ struct drm_atomic_state *state)
+{
+ unsigned long flags;
+ struct vmw_private *vmw = vmw_priv(crtc->dev);
+
+ if (vmw->vkms_enabled && crtc->state->event) {
+ spin_lock_irqsave(&crtc->dev->event_lock, flags);
+
+ if (drm_crtc_vblank_get(crtc) != 0)
+ drm_crtc_send_vblank_event(crtc, crtc->state->event);
+ else
+ drm_crtc_arm_vblank_event(crtc, crtc->state->event);
+
+ spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
+
+ crtc->state->event = NULL;
+ }
+}
+
+void
+vmw_vkms_crtc_atomic_enable(struct drm_crtc *crtc,
+ struct drm_atomic_state *state)
+{
+ struct vmw_private *vmw = vmw_priv(crtc->dev);
+
+ if (vmw->vkms_enabled)
+ drm_crtc_vblank_on(crtc);
+}
+
+void
+vmw_vkms_crtc_atomic_disable(struct drm_crtc *crtc,
+ struct drm_atomic_state *state)
+{
+ struct vmw_private *vmw = vmw_priv(crtc->dev);
+
+ if (vmw->vkms_enabled)
+ drm_crtc_vblank_off(crtc);
+}
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h
new file mode 100644
index 000000000000..0f6d4ab9ebe3
--- /dev/null
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h
@@ -0,0 +1,53 @@
+/* SPDX-License-Identifier: GPL-2.0 OR MIT */
+/**************************************************************************
+ *
+ * Copyright (c) 2024 Broadcom. All Rights Reserved. The term
+ * “Broadcom” refers to Broadcom Inc. and/or its subsidiaries.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sub license, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial portions
+ * of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
+ * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+ * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+ * USE OR OTHER DEALINGS IN THE SOFTWARE.
+ *
+ **************************************************************************/
+
+#ifndef VMWGFX_VKMS_H_
+#define VMWGFX_VKMS_H_
+
+#include <linux/hrtimer_types.h>
+#include <linux/types.h>
+
+struct vmw_private;
+struct drm_crtc;
+struct drm_atomic_state;
+
+void vmw_vkms_init(struct vmw_private *vmw);
+bool vmw_vkms_get_vblank_timestamp(struct drm_crtc *crtc,
+ int *max_error,
+ ktime_t *vblank_time,
+ bool in_vblank_irq);
+int vmw_vkms_enable_vblank(struct drm_crtc *crtc);
+void vmw_vkms_disable_vblank(struct drm_crtc *crtc);
+void vmw_vkms_crtc_atomic_flush(struct drm_crtc *crtc, struct drm_atomic_state *state);
+enum hrtimer_restart vmw_vkms_vblank_simulate(struct hrtimer *timer);
+void vmw_vkms_crtc_atomic_enable(struct drm_crtc *crtc,
+ struct drm_atomic_state *state);
+void vmw_vkms_crtc_atomic_disable(struct drm_crtc *crtc,
+ struct drm_atomic_state *state);
+
+#endif
--
2.40.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/5] drm/vmwgfx: Implement virtual crc generation
2024-04-02 23:28 [PATCH 0/5] drm/vmwgfx: vblank and crc generation support Zack Rusin
2024-04-02 23:28 ` [PATCH 1/5] drm/vmwgfx: Implement virtual kms Zack Rusin
@ 2024-04-02 23:28 ` Zack Rusin
2024-04-03 11:31 ` kernel test robot
2024-04-09 15:08 ` Martin Krastev
2024-04-02 23:28 ` [PATCH 3/5] drm/vmwgfx: Fix prime import/export Zack Rusin
` (2 subsequent siblings)
4 siblings, 2 replies; 16+ messages in thread
From: Zack Rusin @ 2024-04-02 23:28 UTC (permalink / raw)
To: dri-devel
Cc: Broadcom internal kernel review list, ian.forbes, martin.krastev,
maaz.mombasawala, Zack Rusin
crc checksums are used to validate the output. Normally they're part
of the actual display hardware but on virtual stack there's nothing
to automatically generate them.
Implement crc generation for the vmwgfx stack. This works only on
screen targets, where it's possibly to easily make sure that the
guest side contents of the surface matches the host sides output.
Just like the vblank support, crc generation can only be enabled via:
guestinfo.vmwgfx.vkms_enable = "TRUE"
option in the vmx file.
Makes IGT's kms_pipe_crc_basic pass and allows a huge number of other
IGT tests which require CRC generation of the output to actually run
on vmwgfx. Makes it possible to actually validate a lof of the kms and
drm functionality with vmwgfx.
Signed-off-by: Zack Rusin <zack.rusin@broadcom.com>
---
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 1 +
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 2 +
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 31 +-
drivers/gpu/drm/vmwgfx/vmwgfx_kms.h | 15 +-
drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 32 +-
drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 22 +-
drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c | 453 ++++++++++++++++++++++-
drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h | 28 +-
8 files changed, 550 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index e34c48fd25d4..89d3679d2608 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -1198,6 +1198,7 @@ static void vmw_driver_unload(struct drm_device *dev)
vmw_svga_disable(dev_priv);
+ vmw_vkms_cleanup(dev_priv);
vmw_kms_close(dev_priv);
vmw_overlay_close(dev_priv);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
index 4f5d7d13c4aa..ddbceaa31b59 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
@@ -616,6 +616,7 @@ struct vmw_private {
uint32 *devcaps;
bool vkms_enabled;
+ struct workqueue_struct *crc_workq;
/*
* mksGuestStat instance-descriptor and pid arrays
@@ -811,6 +812,7 @@ void vmw_resource_mob_attach(struct vmw_resource *res);
void vmw_resource_mob_detach(struct vmw_resource *res);
void vmw_resource_dirty_update(struct vmw_resource *res, pgoff_t start,
pgoff_t end);
+int vmw_resource_clean(struct vmw_resource *res);
int vmw_resources_clean(struct vmw_bo *vbo, pgoff_t start,
pgoff_t end, pgoff_t *num_prefault);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index e763cf0e6cfc..e33e5993d8fc 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -40,14 +40,14 @@
void vmw_du_init(struct vmw_display_unit *du)
{
- hrtimer_init(&du->vkms.timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
- du->vkms.timer.function = &vmw_vkms_vblank_simulate;
+ vmw_vkms_crtc_init(&du->crtc);
}
void vmw_du_cleanup(struct vmw_display_unit *du)
{
struct vmw_private *dev_priv = vmw_priv(du->primary.dev);
- hrtimer_cancel(&du->vkms.timer);
+
+ vmw_vkms_crtc_cleanup(&du->crtc);
drm_plane_cleanup(&du->primary);
if (vmw_cmd_supported(dev_priv))
drm_plane_cleanup(&du->cursor.base);
@@ -963,6 +963,7 @@ int vmw_du_crtc_atomic_check(struct drm_crtc *crtc,
void vmw_du_crtc_atomic_begin(struct drm_crtc *crtc,
struct drm_atomic_state *state)
{
+ vmw_vkms_crtc_atomic_begin(crtc, state);
}
/**
@@ -2029,6 +2030,29 @@ vmw_kms_create_hotplug_mode_update_property(struct vmw_private *dev_priv)
"hotplug_mode_update", 0, 1);
}
+static void
+vmw_atomic_commit_tail(struct drm_atomic_state *old_state)
+{
+ struct vmw_private *vmw = vmw_priv(old_state->dev);
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *old_crtc_state;
+ int i;
+
+ drm_atomic_helper_commit_tail(old_state);
+
+ if (vmw->vkms_enabled) {
+ for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+ struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
+ (void)old_crtc_state;
+ flush_work(&du->vkms.crc_generator_work);
+ }
+ }
+}
+
+static const struct drm_mode_config_helper_funcs vmw_mode_config_helpers = {
+ .atomic_commit_tail = vmw_atomic_commit_tail,
+};
+
int vmw_kms_init(struct vmw_private *dev_priv)
{
struct drm_device *dev = &dev_priv->drm;
@@ -2048,6 +2072,7 @@ int vmw_kms_init(struct vmw_private *dev_priv)
dev->mode_config.max_width = dev_priv->texture_max_width;
dev->mode_config.max_height = dev_priv->texture_max_height;
dev->mode_config.preferred_depth = dev_priv->assume_16bpp ? 16 : 32;
+ dev->mode_config.helper_private = &vmw_mode_config_helpers;
drm_mode_create_suggested_offset_properties(dev);
vmw_kms_create_hotplug_mode_update_property(dev_priv);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
index 9e83a1553286..bf9931e3a728 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
@@ -378,9 +378,22 @@ struct vmw_display_unit {
int set_gui_y;
struct {
+ struct work_struct crc_generator_work;
struct hrtimer timer;
ktime_t period_ns;
- struct drm_pending_vblank_event *event;
+
+ /* protects concurrent access to the vblank handler */
+ atomic_t atomic_lock;
+ /* protected by @atomic_lock */
+ bool crc_enabled;
+ struct vmw_surface *surface;
+
+ /* protects concurrent access to the crc worker */
+ spinlock_t crc_state_lock;
+ /* protected by @crc_state_lock */
+ bool crc_pending;
+ u64 frame_start;
+ u64 frame_end;
} vkms;
};
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
index ca300c7427d2..848dba09981b 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
@@ -1064,6 +1064,22 @@ void vmw_resource_dirty_update(struct vmw_resource *res, pgoff_t start,
end << PAGE_SHIFT);
}
+int vmw_resource_clean(struct vmw_resource *res)
+{
+ int ret = 0;
+
+ if (res->res_dirty) {
+ if (!res->func->clean)
+ return -EINVAL;
+
+ ret = res->func->clean(res);
+ if (ret)
+ return ret;
+ res->res_dirty = false;
+ }
+ return ret;
+}
+
/**
* vmw_resources_clean - Clean resources intersecting a mob range
* @vbo: The mob buffer object
@@ -1080,6 +1096,7 @@ int vmw_resources_clean(struct vmw_bo *vbo, pgoff_t start,
unsigned long res_start = start << PAGE_SHIFT;
unsigned long res_end = end << PAGE_SHIFT;
unsigned long last_cleaned = 0;
+ int ret;
/*
* Find the resource with lowest backup_offset that intersects the
@@ -1106,18 +1123,9 @@ int vmw_resources_clean(struct vmw_bo *vbo, pgoff_t start,
* intersecting the range.
*/
while (found) {
- if (found->res_dirty) {
- int ret;
-
- if (!found->func->clean)
- return -EINVAL;
-
- ret = found->func->clean(found);
- if (ret)
- return ret;
-
- found->res_dirty = false;
- }
+ ret = vmw_resource_clean(found);
+ if (ret)
+ return ret;
last_cleaned = found->guest_memory_offset + found->guest_memory_size;
cur = rb_next(&found->mob_node);
if (!cur)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
index 665bde7e0be0..2041c4d48daa 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
@@ -409,11 +409,6 @@ static void vmw_stdu_crtc_mode_set_nofb(struct drm_crtc *crtc)
crtc->x, crtc->y);
}
-
-static void vmw_stdu_crtc_helper_prepare(struct drm_crtc *crtc)
-{
-}
-
static void vmw_stdu_crtc_atomic_disable(struct drm_crtc *crtc,
struct drm_atomic_state *state)
{
@@ -783,6 +778,9 @@ static const struct drm_crtc_funcs vmw_stdu_crtc_funcs = {
.enable_vblank = vmw_vkms_enable_vblank,
.disable_vblank = vmw_vkms_disable_vblank,
.get_vblank_timestamp = vmw_vkms_get_vblank_timestamp,
+ .get_crc_sources = vmw_vkms_get_crc_sources,
+ .set_crc_source = vmw_vkms_set_crc_source,
+ .verify_crc_source = vmw_vkms_verify_crc_source,
};
@@ -1414,6 +1412,17 @@ vmw_stdu_primary_plane_atomic_update(struct drm_plane *plane,
vmw_fence_obj_unreference(&fence);
}
+static void
+vmw_stdu_crtc_atomic_flush(struct drm_crtc *crtc,
+ struct drm_atomic_state *state)
+{
+ struct vmw_private *vmw = vmw_priv(crtc->dev);
+ struct vmw_screen_target_display_unit *stdu = vmw_crtc_to_stdu(crtc);
+
+ if (vmw->vkms_enabled)
+ vmw_vkms_set_crc_surface(crtc, stdu->display_srf);
+ vmw_vkms_crtc_atomic_flush(crtc, state);
+}
static const struct drm_plane_funcs vmw_stdu_plane_funcs = {
.update_plane = drm_atomic_helper_update_plane,
@@ -1454,11 +1463,10 @@ drm_plane_helper_funcs vmw_stdu_primary_plane_helper_funcs = {
};
static const struct drm_crtc_helper_funcs vmw_stdu_crtc_helper_funcs = {
- .prepare = vmw_stdu_crtc_helper_prepare,
.mode_set_nofb = vmw_stdu_crtc_mode_set_nofb,
.atomic_check = vmw_du_crtc_atomic_check,
.atomic_begin = vmw_du_crtc_atomic_begin,
- .atomic_flush = vmw_vkms_crtc_atomic_flush,
+ .atomic_flush = vmw_stdu_crtc_atomic_flush,
.atomic_enable = vmw_vkms_crtc_atomic_enable,
.atomic_disable = vmw_stdu_crtc_atomic_disable,
};
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
index ff76bfd70f91..5138a7107897 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
@@ -28,33 +28,179 @@
#include "vmwgfx_vkms.h"
+#include "vmwgfx_bo.h"
#include "vmwgfx_drv.h"
#include "vmwgfx_kms.h"
#include "vmwgfx_vkms.h"
+#include "vmw_surface_cache.h"
+
#include <drm/drm_crtc.h>
+#include <drm/drm_debugfs_crc.h>
#include <drm/drm_print.h>
#include <drm/drm_vblank.h>
+#include <linux/crc32.h>
+#include <linux/delay.h>
+
#define GUESTINFO_VBLANK "guestinfo.vmwgfx.vkms_enable"
-enum hrtimer_restart
+static int
+vmw_surface_sync(struct vmw_private *vmw,
+ struct vmw_surface *surf)
+{
+ int ret;
+ struct vmw_fence_obj *fence = NULL;
+ struct vmw_bo *bo = surf->res.guest_memory_bo;
+
+ vmw_resource_clean(&surf->res);
+
+ ret = ttm_bo_reserve(&bo->tbo, false, false, NULL);
+ if (ret != 0) {
+ drm_warn(&vmw->drm, "%s: failed reserve\n", __func__);
+ goto done;
+ }
+
+ ret = vmw_execbuf_fence_commands(NULL, vmw, &fence, NULL);
+ if (ret != 0) {
+ drm_warn(&vmw->drm, "%s: failed execbuf\n", __func__);
+ ttm_bo_unreserve(&bo->tbo);
+ goto done;
+ }
+
+ dma_fence_wait(&fence->base, false);
+ dma_fence_put(&fence->base);
+
+ ttm_bo_unreserve(&bo->tbo);
+done:
+ return ret;
+}
+
+static int
+compute_crc(struct drm_crtc *crtc,
+ struct vmw_surface *surf,
+ u32 *crc)
+{
+ u8 *mapped_surface;
+ struct vmw_bo *bo = surf->res.guest_memory_bo;
+ const struct SVGA3dSurfaceDesc *desc =
+ vmw_surface_get_desc(surf->metadata.format);
+ u32 row_pitch_bytes;
+ SVGA3dSize blocks;
+ u32 y;
+
+ *crc = 0;
+
+ vmw_surface_get_size_in_blocks(desc, &surf->metadata.base_size, &blocks);
+ row_pitch_bytes = blocks.width * desc->pitchBytesPerBlock;
+ WARN_ON(!bo);
+ mapped_surface = vmw_bo_map_and_cache(bo);
+
+ for (y = 0; y < blocks.height; y++) {
+ *crc = crc32_le(*crc, mapped_surface, row_pitch_bytes);
+ mapped_surface += row_pitch_bytes;
+ }
+
+ vmw_bo_unmap(bo);
+
+ return 0;
+}
+
+static void
+crc_generate_worker(struct work_struct *work)
+{
+ struct vmw_display_unit *du =
+ container_of(work, struct vmw_display_unit, vkms.crc_generator_work);
+ struct drm_crtc *crtc = &du->crtc;
+ struct vmw_private *vmw = vmw_priv(crtc->dev);
+ bool crc_pending;
+ u64 frame_start, frame_end;
+ u32 crc32 = 0;
+ struct vmw_surface *surf = 0;
+ int ret;
+
+ spin_lock_irq(&du->vkms.crc_state_lock);
+ crc_pending = du->vkms.crc_pending;
+ spin_unlock_irq(&du->vkms.crc_state_lock);
+
+ /*
+ * We raced with the vblank hrtimer and previous work already computed
+ * the crc, nothing to do.
+ */
+ if (!crc_pending)
+ return;
+
+ spin_lock_irq(&du->vkms.crc_state_lock);
+ surf = du->vkms.surface;
+ spin_unlock_irq(&du->vkms.crc_state_lock);
+
+ if (vmw_surface_sync(vmw, surf)) {
+ drm_warn(crtc->dev, "CRC worker wasn't able to sync the crc surface!\n");
+ return;
+ }
+
+ ret = compute_crc(crtc, surf, &crc32);
+ if (ret)
+ return;
+
+ spin_lock_irq(&du->vkms.crc_state_lock);
+ frame_start = du->vkms.frame_start;
+ frame_end = du->vkms.frame_end;
+ crc_pending = du->vkms.crc_pending;
+ du->vkms.frame_start = 0;
+ du->vkms.frame_end = 0;
+ du->vkms.crc_pending = false;
+ spin_unlock_irq(&du->vkms.crc_state_lock);
+
+ /*
+ * The worker can fall behind the vblank hrtimer, make sure we catch up.
+ */
+ while (frame_start <= frame_end)
+ drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
+}
+
+static enum hrtimer_restart
vmw_vkms_vblank_simulate(struct hrtimer *timer)
{
struct vmw_display_unit *du = container_of(timer, struct vmw_display_unit, vkms.timer);
struct drm_crtc *crtc = &du->crtc;
+ struct vmw_private *vmw = vmw_priv(crtc->dev);
+ struct vmw_surface *surf = NULL;
u64 ret_overrun;
- bool ret;
+ bool locked, ret;
ret_overrun = hrtimer_forward_now(&du->vkms.timer,
du->vkms.period_ns);
if (ret_overrun != 1)
- DRM_WARN("%s: vblank timer overrun\n", __func__);
+ drm_dbg_driver(crtc->dev, "vblank timer missed %lld frames.\n",
+ ret_overrun - 1);
+ locked = vmw_vkms_vblank_trylock(crtc);
ret = drm_crtc_handle_vblank(crtc);
- /* Don't queue timer again when vblank is disabled. */
- if (!ret)
- return HRTIMER_NORESTART;
+ WARN_ON(!ret);
+ if (!locked)
+ return HRTIMER_RESTART;
+ surf = du->vkms.surface;
+ vmw_vkms_unlock(crtc);
+
+ if (du->vkms.crc_enabled && surf) {
+ u64 frame = drm_crtc_accurate_vblank_count(crtc);
+
+ spin_lock(&du->vkms.crc_state_lock);
+ if (!du->vkms.crc_pending)
+ du->vkms.frame_start = frame;
+ else
+ drm_dbg_driver(crtc->dev,
+ "crc worker falling behind, frame_start: %llu, frame_end: %llu\n",
+ du->vkms.frame_start, frame);
+ du->vkms.frame_end = frame;
+ du->vkms.crc_pending = true;
+ spin_unlock(&du->vkms.crc_state_lock);
+
+ ret = queue_work(vmw->crc_workq, &du->vkms.crc_generator_work);
+ if (!ret)
+ drm_dbg_driver(crtc->dev, "Composer worker already queued\n");
+ }
return HRTIMER_RESTART;
}
@@ -78,8 +224,21 @@ vmw_vkms_init(struct vmw_private *vmw)
if (!ret && vmw->vkms_enabled) {
ret = drm_vblank_init(&vmw->drm, VMWGFX_NUM_DISPLAY_UNITS);
vmw->vkms_enabled = (ret == 0);
- drm_info(&vmw->drm, "vkms_enabled = %d\n", vmw->vkms_enabled);
}
+
+ vmw->crc_workq = alloc_ordered_workqueue("vmwgfx_crc_generator", 0);
+ if (!vmw->crc_workq) {
+ drm_warn(&vmw->drm, "crc workqueue allocation failed. Disabling vkms.");
+ vmw->vkms_enabled = false;
+ }
+ if (vmw->vkms_enabled)
+ drm_info(&vmw->drm, "VKMS enabled\n");
+}
+
+void
+vmw_vkms_cleanup(struct vmw_private *vmw)
+{
+ destroy_workqueue(vmw->crc_workq);
}
bool
@@ -133,6 +292,8 @@ vmw_vkms_enable_vblank(struct drm_crtc *crtc)
drm_calc_timestamping_constants(crtc, &crtc->mode);
+ hrtimer_init(&du->vkms.timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+ du->vkms.timer.function = &vmw_vkms_vblank_simulate;
du->vkms.period_ns = ktime_set(0, vblank->framedur_ns);
hrtimer_start(&du->vkms.timer, du->vkms.period_ns, HRTIMER_MODE_REL);
@@ -148,7 +309,46 @@ vmw_vkms_disable_vblank(struct drm_crtc *crtc)
if (!vmw->vkms_enabled)
return;
- hrtimer_try_to_cancel(&du->vkms.timer);
+ hrtimer_cancel(&du->vkms.timer);
+ du->vkms.surface = NULL;
+ du->vkms.period_ns = ktime_set(0, 0);
+}
+
+enum vmw_vkms_lock_state {
+ VMW_VKMS_LOCK_UNLOCKED = 0,
+ VMW_VKMS_LOCK_MODESET = 1,
+ VMW_VKMS_LOCK_VBLANK = 2
+};
+
+void
+vmw_vkms_crtc_init(struct drm_crtc *crtc)
+{
+ struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
+
+ atomic_set(&du->vkms.atomic_lock, VMW_VKMS_LOCK_UNLOCKED);
+ spin_lock_init(&du->vkms.crc_state_lock);
+
+ INIT_WORK(&du->vkms.crc_generator_work, crc_generate_worker);
+ du->vkms.surface = NULL;
+}
+
+void
+vmw_vkms_crtc_cleanup(struct drm_crtc *crtc)
+{
+ struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
+
+ WARN_ON(work_pending(&du->vkms.crc_generator_work));
+ hrtimer_cancel(&du->vkms.timer);
+}
+
+void
+vmw_vkms_crtc_atomic_begin(struct drm_crtc *crtc,
+ struct drm_atomic_state *state)
+{
+ struct vmw_private *vmw = vmw_priv(crtc->dev);
+
+ if (vmw->vkms_enabled)
+ vmw_vkms_modeset_lock(crtc);
}
void
@@ -170,6 +370,9 @@ vmw_vkms_crtc_atomic_flush(struct drm_crtc *crtc,
crtc->state->event = NULL;
}
+
+ if (vmw->vkms_enabled)
+ vmw_vkms_unlock(crtc);
}
void
@@ -191,3 +394,237 @@ vmw_vkms_crtc_atomic_disable(struct drm_crtc *crtc,
if (vmw->vkms_enabled)
drm_crtc_vblank_off(crtc);
}
+
+static bool
+is_crc_supported(struct drm_crtc *crtc)
+{
+ struct vmw_private *vmw = vmw_priv(crtc->dev);
+
+ if (!vmw->vkms_enabled)
+ return false;
+
+ if (vmw->active_display_unit != vmw_du_screen_target)
+ return false;
+
+ return true;
+}
+
+static const char * const pipe_crc_sources[] = {"auto"};
+
+static int
+crc_parse_source(const char *src_name,
+ bool *enabled)
+{
+ int ret = 0;
+
+ if (!src_name) {
+ *enabled = false;
+ } else if (strcmp(src_name, "auto") == 0) {
+ *enabled = true;
+ } else {
+ *enabled = false;
+ ret = -EINVAL;
+ }
+
+ return ret;
+}
+
+const char *const *
+vmw_vkms_get_crc_sources(struct drm_crtc *crtc,
+ size_t *count)
+{
+ *count = 0;
+ if (!is_crc_supported(crtc))
+ return NULL;
+
+ *count = ARRAY_SIZE(pipe_crc_sources);
+ return pipe_crc_sources;
+}
+
+int
+vmw_vkms_verify_crc_source(struct drm_crtc *crtc,
+ const char *src_name,
+ size_t *values_cnt)
+{
+ bool enabled;
+
+ if (!is_crc_supported(crtc))
+ return -EINVAL;
+
+ if (crc_parse_source(src_name, &enabled) < 0) {
+ drm_dbg_driver(crtc->dev, "unknown source '%s'\n", src_name);
+ return -EINVAL;
+ }
+
+ *values_cnt = 1;
+
+ return 0;
+}
+
+int
+vmw_vkms_set_crc_source(struct drm_crtc *crtc,
+ const char *src_name)
+{
+ struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
+ bool enabled, prev_enabled, locked;
+ int ret;
+
+ if (!is_crc_supported(crtc))
+ return -EINVAL;
+
+ ret = crc_parse_source(src_name, &enabled);
+
+ if (enabled)
+ drm_crtc_vblank_get(crtc);
+
+ locked = vmw_vkms_modeset_lock_relaxed(crtc);
+ prev_enabled = du->vkms.crc_enabled;
+ du->vkms.crc_enabled = enabled;
+ if (locked)
+ vmw_vkms_unlock(crtc);
+
+ if (prev_enabled)
+ drm_crtc_vblank_put(crtc);
+
+ return ret;
+}
+
+void
+vmw_vkms_set_crc_surface(struct drm_crtc *crtc,
+ struct vmw_surface *surf)
+{
+ struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
+ struct vmw_private *vmw = vmw_priv(crtc->dev);
+
+ if (vmw->vkms_enabled) {
+ WARN_ON(atomic_read(&du->vkms.atomic_lock) != VMW_VKMS_LOCK_MODESET);
+ du->vkms.surface = surf;
+ }
+}
+
+/**
+ * vmw_vkms_lock_max_delay - Return the max wait for the vkms lock
+ * @du: The vmw_display_unit from which to grab the vblank timings
+ *
+ * Returns the maximum wait time used to acquire the vkms lock. By
+ * default uses a time of a single frame and in case where vblank
+ * was not initialized for the display unit 1/60th of a second.
+ */
+static inline u64
+vmw_vkms_lock_max_wait_ns(struct vmw_display_unit *du)
+{
+ s64 nsecs = ktime_to_ns(du->vkms.period_ns);
+
+ return (nsecs > 0) ? nsecs : 16666666;
+}
+
+/**
+ * vmw_vkms_modeset_lock - Protects access to crtc during modeset
+ * @crtc: The crtc to lock for vkms
+ *
+ * This function prevents the VKMS timers/callbacks from being called
+ * while a modeset operation is in process. We don't want the callbacks
+ * e.g. the vblank simulator to be trying to access incomplete state
+ * so we need to make sure they execute only when the modeset has
+ * finished.
+ *
+ * Normally this would have been done with a spinlock but locking the
+ * entire atomic modeset with vmwgfx is impossible because kms prepare
+ * executes non-atomic ops (e.g. vmw_validation_prepare holds a mutex to
+ * guard various bits of state). Which means that we need to synchronize
+ * atomic context (the vblank handler) with the non-atomic entirity
+ * of kms - so use an atomic_t to track which part of vkms has access
+ * to the basic vkms state.
+ */
+void
+vmw_vkms_modeset_lock(struct drm_crtc *crtc)
+{
+ struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
+ const u64 nsecs_delay = 10;
+ const u64 MAX_NSECS_DELAY = vmw_vkms_lock_max_wait_ns(du);
+ u64 total_delay = 0;
+ int ret;
+
+ do {
+ ret = atomic_cmpxchg(&du->vkms.atomic_lock,
+ VMW_VKMS_LOCK_UNLOCKED,
+ VMW_VKMS_LOCK_MODESET);
+ if (ret == VMW_VKMS_LOCK_UNLOCKED || total_delay >= MAX_NSECS_DELAY)
+ break;
+ ndelay(nsecs_delay);
+ total_delay += nsecs_delay;
+ } while (1);
+
+ if (total_delay >= MAX_NSECS_DELAY) {
+ drm_warn(crtc->dev, "VKMS lock expired! total_delay = %lld, ret = %d, cur = %d\n",
+ total_delay, ret, atomic_read(&du->vkms.atomic_lock));
+ }
+}
+
+/**
+ * vmw_vkms_modeset_lock_relaxed - Protects access to crtc during modeset
+ * @crtc: The crtc to lock for vkms
+ *
+ * Much like vmw_vkms_modeset_lock except that when the crtc is currently
+ * in a modeset it will return immediately.
+ *
+ * Returns true if actually locked vkms to modeset or false otherwise.
+ */
+bool
+vmw_vkms_modeset_lock_relaxed(struct drm_crtc *crtc)
+{
+ struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
+ const u64 nsecs_delay = 10;
+ const u64 MAX_NSECS_DELAY = vmw_vkms_lock_max_wait_ns(du);
+ u64 total_delay = 0;
+ int ret;
+
+ do {
+ ret = atomic_cmpxchg(&du->vkms.atomic_lock,
+ VMW_VKMS_LOCK_UNLOCKED,
+ VMW_VKMS_LOCK_MODESET);
+ if (ret == VMW_VKMS_LOCK_UNLOCKED ||
+ ret == VMW_VKMS_LOCK_MODESET ||
+ total_delay >= MAX_NSECS_DELAY)
+ break;
+ ndelay(nsecs_delay);
+ total_delay += nsecs_delay;
+ } while (1);
+
+ if (total_delay >= MAX_NSECS_DELAY) {
+ drm_warn(crtc->dev, "VKMS relaxed lock expired!\n");
+ return false;
+ }
+
+ return ret == VMW_VKMS_LOCK_UNLOCKED;
+}
+
+/**
+ * vmw_vkms_vblank_trylock - Protects access to crtc during vblank
+ * @crtc: The crtc to lock for vkms
+ *
+ * Tries to lock vkms for vblank, returns immediately.
+ *
+ * Returns true if locked vkms to vblank or false otherwise.
+ */
+bool
+vmw_vkms_vblank_trylock(struct drm_crtc *crtc)
+{
+ struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
+ u32 ret;
+
+ ret = atomic_cmpxchg(&du->vkms.atomic_lock,
+ VMW_VKMS_LOCK_UNLOCKED,
+ VMW_VKMS_LOCK_VBLANK);
+
+ return ret == VMW_VKMS_LOCK_UNLOCKED;
+}
+
+void
+vmw_vkms_unlock(struct drm_crtc *crtc)
+{
+ struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
+
+ /* Release flag; mark it as unlocked. */
+ atomic_set(&du->vkms.atomic_lock, VMW_VKMS_LOCK_UNLOCKED);
+}
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h
index 0f6d4ab9ebe3..69ddd33a8444 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h
@@ -32,22 +32,44 @@
#include <linux/hrtimer_types.h>
#include <linux/types.h>
-struct vmw_private;
-struct drm_crtc;
struct drm_atomic_state;
+struct drm_crtc;
+struct vmw_private;
+struct vmw_surface;
void vmw_vkms_init(struct vmw_private *vmw);
+void vmw_vkms_cleanup(struct vmw_private *vmw);
+
+void vmw_vkms_modeset_lock(struct drm_crtc *crtc);
+bool vmw_vkms_modeset_lock_relaxed(struct drm_crtc *crtc);
+bool vmw_vkms_vblank_trylock(struct drm_crtc *crtc);
+void vmw_vkms_unlock(struct drm_crtc *crtc);
+
bool vmw_vkms_get_vblank_timestamp(struct drm_crtc *crtc,
int *max_error,
ktime_t *vblank_time,
bool in_vblank_irq);
int vmw_vkms_enable_vblank(struct drm_crtc *crtc);
void vmw_vkms_disable_vblank(struct drm_crtc *crtc);
+
+void vmw_vkms_crtc_init(struct drm_crtc *crtc);
+void vmw_vkms_crtc_cleanup(struct drm_crtc *crtc);
+void vmw_vkms_crtc_atomic_begin(struct drm_crtc *crtc,
+ struct drm_atomic_state *state);
void vmw_vkms_crtc_atomic_flush(struct drm_crtc *crtc, struct drm_atomic_state *state);
-enum hrtimer_restart vmw_vkms_vblank_simulate(struct hrtimer *timer);
void vmw_vkms_crtc_atomic_enable(struct drm_crtc *crtc,
struct drm_atomic_state *state);
void vmw_vkms_crtc_atomic_disable(struct drm_crtc *crtc,
struct drm_atomic_state *state);
+const char *const *vmw_vkms_get_crc_sources(struct drm_crtc *crtc,
+ size_t *count);
+int vmw_vkms_verify_crc_source(struct drm_crtc *crtc,
+ const char *src_name,
+ size_t *values_cnt);
+int vmw_vkms_set_crc_source(struct drm_crtc *crtc,
+ const char *src_name);
+void vmw_vkms_set_crc_surface(struct drm_crtc *crtc,
+ struct vmw_surface *surf);
+
#endif
--
2.40.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/5] drm/vmwgfx: Fix prime import/export
2024-04-02 23:28 [PATCH 0/5] drm/vmwgfx: vblank and crc generation support Zack Rusin
2024-04-02 23:28 ` [PATCH 1/5] drm/vmwgfx: Implement virtual kms Zack Rusin
2024-04-02 23:28 ` [PATCH 2/5] drm/vmwgfx: Implement virtual crc generation Zack Rusin
@ 2024-04-02 23:28 ` Zack Rusin
2024-04-09 15:30 ` Martin Krastev
2024-04-02 23:28 ` [PATCH 4/5] drm/vmwgfx: Fix crtc's atomic check conditional Zack Rusin
2024-04-02 23:28 ` [PATCH 5/5] drm/vmwgfx: Sort primary plane formats by order of preference Zack Rusin
4 siblings, 1 reply; 16+ messages in thread
From: Zack Rusin @ 2024-04-02 23:28 UTC (permalink / raw)
To: dri-devel
Cc: Broadcom internal kernel review list, ian.forbes, martin.krastev,
maaz.mombasawala, Zack Rusin, stable
vmwgfx never supported prime import of external buffers. Furthermore the
driver exposes two different objects to userspace: vmw_surface's and
gem buffers but prime import/export only worked with vmw_surfaces.
Because gem buffers are used through the dumb_buffer interface this meant
that the driver created buffers couldn't have been prime exported or
imported.
Fix prime import/export. Makes IGT's kms_prime pass.
Signed-off-by: Zack Rusin <zack.rusin@broadcom.com>
Fixes: 8afa13a0583f ("drm/vmwgfx: Implement DRIVER_GEM")
Cc: <stable@vger.kernel.org> # v6.6+
---
drivers/gpu/drm/vmwgfx/vmwgfx_blit.c | 35 +++++++++++++++--
drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 7 ++--
drivers/gpu/drm/vmwgfx/vmwgfx_bo.h | 2 +
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 1 +
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 3 ++
drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 32 ++++++++++++++++
drivers/gpu/drm/vmwgfx/vmwgfx_prime.c | 15 +++++++-
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 44 +++++++++++++++-------
8 files changed, 117 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_blit.c b/drivers/gpu/drm/vmwgfx/vmwgfx_blit.c
index c52c7bf1485b..717d624e9a05 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_blit.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_blit.c
@@ -456,8 +456,10 @@ int vmw_bo_cpu_blit(struct ttm_buffer_object *dst,
.no_wait_gpu = false
};
u32 j, initial_line = dst_offset / dst_stride;
- struct vmw_bo_blit_line_data d;
+ struct vmw_bo_blit_line_data d = {0};
int ret = 0;
+ struct page **dst_pages = NULL;
+ struct page **src_pages = NULL;
/* Buffer objects need to be either pinned or reserved: */
if (!(dst->pin_count))
@@ -477,12 +479,35 @@ int vmw_bo_cpu_blit(struct ttm_buffer_object *dst,
return ret;
}
+ if (!src->ttm->pages && src->ttm->sg) {
+ src_pages = kvmalloc_array(src->ttm->num_pages,
+ sizeof(struct page *), GFP_KERNEL);
+ if (!src_pages)
+ return -ENOMEM;
+ ret = drm_prime_sg_to_page_array(src->ttm->sg, src_pages,
+ src->ttm->num_pages);
+ if (ret)
+ goto out;
+ }
+ if (!dst->ttm->pages && dst->ttm->sg) {
+ dst_pages = kvmalloc_array(dst->ttm->num_pages,
+ sizeof(struct page *), GFP_KERNEL);
+ if (!dst_pages) {
+ ret = -ENOMEM;
+ goto out;
+ }
+ ret = drm_prime_sg_to_page_array(dst->ttm->sg, dst_pages,
+ dst->ttm->num_pages);
+ if (ret)
+ goto out;
+ }
+
d.mapped_dst = 0;
d.mapped_src = 0;
d.dst_addr = NULL;
d.src_addr = NULL;
- d.dst_pages = dst->ttm->pages;
- d.src_pages = src->ttm->pages;
+ d.dst_pages = dst->ttm->pages ? dst->ttm->pages : dst_pages;
+ d.src_pages = src->ttm->pages ? src->ttm->pages : src_pages;
d.dst_num_pages = PFN_UP(dst->resource->size);
d.src_num_pages = PFN_UP(src->resource->size);
d.dst_prot = ttm_io_prot(dst, dst->resource, PAGE_KERNEL);
@@ -504,6 +529,10 @@ int vmw_bo_cpu_blit(struct ttm_buffer_object *dst,
kunmap_atomic(d.src_addr);
if (d.dst_addr)
kunmap_atomic(d.dst_addr);
+ if (src_pages)
+ kvfree(src_pages);
+ if (dst_pages)
+ kvfree(dst_pages);
return ret;
}
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
index bfd41ce3c8f4..e5eb21a471a6 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
@@ -377,7 +377,8 @@ static int vmw_bo_init(struct vmw_private *dev_priv,
{
struct ttm_operation_ctx ctx = {
.interruptible = params->bo_type != ttm_bo_type_kernel,
- .no_wait_gpu = false
+ .no_wait_gpu = false,
+ .resv = params->resv,
};
struct ttm_device *bdev = &dev_priv->bdev;
struct drm_device *vdev = &dev_priv->drm;
@@ -394,8 +395,8 @@ static int vmw_bo_init(struct vmw_private *dev_priv,
vmw_bo_placement_set(vmw_bo, params->domain, params->busy_domain);
ret = ttm_bo_init_reserved(bdev, &vmw_bo->tbo, params->bo_type,
- &vmw_bo->placement, 0, &ctx, NULL,
- NULL, destroy);
+ &vmw_bo->placement, 0, &ctx,
+ params->sg, params->resv, destroy);
if (unlikely(ret))
return ret;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.h b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.h
index 0d496dc9c6af..f349642e6190 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.h
@@ -55,6 +55,8 @@ struct vmw_bo_params {
enum ttm_bo_type bo_type;
size_t size;
bool pin;
+ struct dma_resv *resv;
+ struct sg_table *sg;
};
/**
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 89d3679d2608..41ad13e45554 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -1631,6 +1631,7 @@ static const struct drm_driver driver = {
.prime_fd_to_handle = vmw_prime_fd_to_handle,
.prime_handle_to_fd = vmw_prime_handle_to_fd,
+ .gem_prime_import_sg_table = vmw_prime_import_sg_table,
.fops = &vmwgfx_driver_fops,
.name = VMWGFX_DRIVER_NAME,
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
index ddbceaa31b59..4ecaea0026fc 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
@@ -1107,6 +1107,9 @@ extern int vmw_prime_handle_to_fd(struct drm_device *dev,
struct drm_file *file_priv,
uint32_t handle, uint32_t flags,
int *prime_fd);
+struct drm_gem_object *vmw_prime_import_sg_table(struct drm_device *dev,
+ struct dma_buf_attachment *attach,
+ struct sg_table *table);
/*
* MemoryOBject management - vmwgfx_mob.c
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
index 186150f41fbc..2132a8ad8c0c 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
@@ -136,6 +136,38 @@ int vmw_gem_object_create_with_handle(struct vmw_private *dev_priv,
return ret;
}
+struct drm_gem_object *vmw_prime_import_sg_table(struct drm_device *dev,
+ struct dma_buf_attachment *attach,
+ struct sg_table *table)
+{
+ int ret;
+ struct vmw_private *dev_priv = vmw_priv(dev);
+ struct drm_gem_object *gem = NULL;
+ struct vmw_bo *vbo;
+ struct vmw_bo_params params = {
+ .domain = (dev_priv->has_mob) ? VMW_BO_DOMAIN_SYS : VMW_BO_DOMAIN_VRAM,
+ .busy_domain = VMW_BO_DOMAIN_SYS,
+ .bo_type = ttm_bo_type_sg,
+ .size = attach->dmabuf->size,
+ .pin = false,
+ .resv = attach->dmabuf->resv,
+ .sg = table,
+
+ };
+
+ dma_resv_lock(params.resv, NULL);
+
+ ret = vmw_bo_create(dev_priv, ¶ms, &vbo);
+ if (ret != 0)
+ goto out_no_bo;
+
+ vbo->tbo.base.funcs = &vmw_gem_object_funcs;
+
+ gem = &vbo->tbo.base;
+out_no_bo:
+ dma_resv_unlock(params.resv);
+ return gem;
+}
int vmw_gem_object_create_ioctl(struct drm_device *dev, void *data,
struct drm_file *filp)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c b/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c
index 2d72a5ee7c0c..c99cad444991 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c
@@ -75,8 +75,12 @@ int vmw_prime_fd_to_handle(struct drm_device *dev,
int fd, u32 *handle)
{
struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile;
+ int ret = ttm_prime_fd_to_handle(tfile, fd, handle);
- return ttm_prime_fd_to_handle(tfile, fd, handle);
+ if (ret)
+ ret = drm_gem_prime_fd_to_handle(dev, file_priv, fd, handle);
+
+ return ret;
}
int vmw_prime_handle_to_fd(struct drm_device *dev,
@@ -85,5 +89,12 @@ int vmw_prime_handle_to_fd(struct drm_device *dev,
int *prime_fd)
{
struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile;
- return ttm_prime_handle_to_fd(tfile, handle, flags, prime_fd);
+ int ret;
+
+ if (handle > VMWGFX_NUM_MOB)
+ ret = ttm_prime_handle_to_fd(tfile, handle, flags, prime_fd);
+ else
+ ret = drm_gem_prime_handle_to_fd(dev, file_priv, handle, flags, prime_fd);
+
+ return ret;
}
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
index 4d23d0a70bcb..621d98b376bb 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
@@ -188,13 +188,18 @@ static int vmw_ttm_map_dma(struct vmw_ttm_tt *vmw_tt)
switch (dev_priv->map_mode) {
case vmw_dma_map_bind:
case vmw_dma_map_populate:
- vsgt->sgt = &vmw_tt->sgt;
- ret = sg_alloc_table_from_pages_segment(
- &vmw_tt->sgt, vsgt->pages, vsgt->num_pages, 0,
- (unsigned long)vsgt->num_pages << PAGE_SHIFT,
- dma_get_max_seg_size(dev_priv->drm.dev), GFP_KERNEL);
- if (ret)
- goto out_sg_alloc_fail;
+ if (vmw_tt->dma_ttm.page_flags & TTM_TT_FLAG_EXTERNAL) {
+ vsgt->sgt = vmw_tt->dma_ttm.sg;
+ } else {
+ vsgt->sgt = &vmw_tt->sgt;
+ ret = sg_alloc_table_from_pages_segment(&vmw_tt->sgt,
+ vsgt->pages, vsgt->num_pages, 0,
+ (unsigned long)vsgt->num_pages << PAGE_SHIFT,
+ dma_get_max_seg_size(dev_priv->drm.dev),
+ GFP_KERNEL);
+ if (ret)
+ goto out_sg_alloc_fail;
+ }
ret = vmw_ttm_map_for_dma(vmw_tt);
if (unlikely(ret != 0))
@@ -209,8 +214,9 @@ static int vmw_ttm_map_dma(struct vmw_ttm_tt *vmw_tt)
return 0;
out_map_fail:
- sg_free_table(vmw_tt->vsgt.sgt);
- vmw_tt->vsgt.sgt = NULL;
+ drm_warn(&dev_priv->drm, "VSG table map failed!");
+ sg_free_table(vsgt->sgt);
+ vsgt->sgt = NULL;
out_sg_alloc_fail:
return ret;
}
@@ -356,15 +362,17 @@ static void vmw_ttm_destroy(struct ttm_device *bdev, struct ttm_tt *ttm)
static int vmw_ttm_populate(struct ttm_device *bdev,
struct ttm_tt *ttm, struct ttm_operation_ctx *ctx)
{
- int ret;
+ bool external = (ttm->page_flags & TTM_TT_FLAG_EXTERNAL) != 0;
- /* TODO: maybe completely drop this ? */
if (ttm_tt_is_populated(ttm))
return 0;
- ret = ttm_pool_alloc(&bdev->pool, ttm, ctx);
+ if (external && ttm->sg)
+ return drm_prime_sg_to_dma_addr_array(ttm->sg,
+ ttm->dma_address,
+ ttm->num_pages);
- return ret;
+ return ttm_pool_alloc(&bdev->pool, ttm, ctx);
}
static void vmw_ttm_unpopulate(struct ttm_device *bdev,
@@ -372,6 +380,10 @@ static void vmw_ttm_unpopulate(struct ttm_device *bdev,
{
struct vmw_ttm_tt *vmw_tt = container_of(ttm, struct vmw_ttm_tt,
dma_ttm);
+ bool external = (ttm->page_flags & TTM_TT_FLAG_EXTERNAL) != 0;
+
+ if (external)
+ return;
vmw_ttm_unbind(bdev, ttm);
@@ -390,6 +402,7 @@ static struct ttm_tt *vmw_ttm_tt_create(struct ttm_buffer_object *bo,
{
struct vmw_ttm_tt *vmw_be;
int ret;
+ bool external = bo->type == ttm_bo_type_sg;
vmw_be = kzalloc(sizeof(*vmw_be), GFP_KERNEL);
if (!vmw_be)
@@ -398,7 +411,10 @@ static struct ttm_tt *vmw_ttm_tt_create(struct ttm_buffer_object *bo,
vmw_be->dev_priv = vmw_priv_from_ttm(bo->bdev);
vmw_be->mob = NULL;
- if (vmw_be->dev_priv->map_mode == vmw_dma_alloc_coherent)
+ if (external)
+ page_flags |= TTM_TT_FLAG_EXTERNAL | TTM_TT_FLAG_EXTERNAL_MAPPABLE;
+
+ if (vmw_be->dev_priv->map_mode == vmw_dma_alloc_coherent || external)
ret = ttm_sg_tt_init(&vmw_be->dma_ttm, bo, page_flags,
ttm_cached);
else
--
2.40.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 4/5] drm/vmwgfx: Fix crtc's atomic check conditional
2024-04-02 23:28 [PATCH 0/5] drm/vmwgfx: vblank and crc generation support Zack Rusin
` (2 preceding siblings ...)
2024-04-02 23:28 ` [PATCH 3/5] drm/vmwgfx: Fix prime import/export Zack Rusin
@ 2024-04-02 23:28 ` Zack Rusin
2024-04-05 19:02 ` Ian Forbes
2024-04-09 15:12 ` Martin Krastev
2024-04-02 23:28 ` [PATCH 5/5] drm/vmwgfx: Sort primary plane formats by order of preference Zack Rusin
4 siblings, 2 replies; 16+ messages in thread
From: Zack Rusin @ 2024-04-02 23:28 UTC (permalink / raw)
To: dri-devel
Cc: Broadcom internal kernel review list, ian.forbes, martin.krastev,
maaz.mombasawala, Zack Rusin, stable
The conditional was supposed to prevent enabling of a crtc state
without a set primary plane. Accidently it also prevented disabling
crtc state with a set primary plane. Neither is correct.
Fix the conditional and just driver-warn when a crtc state has been
enabled without a primary plane which will help debug broken userspace.
Fixes IGT's kms_atomic_interruptible and kms_atomic_transition tests.
Signed-off-by: Zack Rusin <zack.rusin@broadcom.com>
Fixes: 06ec41909e31 ("drm/vmwgfx: Add and connect CRTC helper functions")
Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v4.12+
---
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index e33e5993d8fc..13b2820cae51 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -931,6 +931,7 @@ int vmw_du_cursor_plane_atomic_check(struct drm_plane *plane,
int vmw_du_crtc_atomic_check(struct drm_crtc *crtc,
struct drm_atomic_state *state)
{
+ struct vmw_private *vmw = vmw_priv(crtc->dev);
struct drm_crtc_state *new_state = drm_atomic_get_new_crtc_state(state,
crtc);
struct vmw_display_unit *du = vmw_crtc_to_du(new_state->crtc);
@@ -938,9 +939,13 @@ int vmw_du_crtc_atomic_check(struct drm_crtc *crtc,
bool has_primary = new_state->plane_mask &
drm_plane_mask(crtc->primary);
- /* We always want to have an active plane with an active CRTC */
- if (has_primary != new_state->enable)
- return -EINVAL;
+ /*
+ * This is fine in general, but broken userspace might expect
+ * some actual rendering so give a clue as why it's blank.
+ */
+ if (new_state->enable && !has_primary)
+ drm_dbg_driver(&vmw->drm,
+ "CRTC without a primary plane will be blank.\n");
if (new_state->connector_mask != connector_mask &&
--
2.40.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 5/5] drm/vmwgfx: Sort primary plane formats by order of preference
2024-04-02 23:28 [PATCH 0/5] drm/vmwgfx: vblank and crc generation support Zack Rusin
` (3 preceding siblings ...)
2024-04-02 23:28 ` [PATCH 4/5] drm/vmwgfx: Fix crtc's atomic check conditional Zack Rusin
@ 2024-04-02 23:28 ` Zack Rusin
2024-04-03 7:42 ` Pekka Paalanen
4 siblings, 1 reply; 16+ messages in thread
From: Zack Rusin @ 2024-04-02 23:28 UTC (permalink / raw)
To: dri-devel
Cc: Broadcom internal kernel review list, ian.forbes, martin.krastev,
maaz.mombasawala, Zack Rusin, stable
The table of primary plane formats wasn't sorted at all, leading to
applications picking our least desirable formats by defaults.
Sort the primary plane formats according to our order of preference.
Fixes IGT's kms_atomic plane-invalid-params which assumes that the
preferred format is a 32bpp format.
Signed-off-by: Zack Rusin <zack.rusin@broadcom.com>
Fixes: 36cc79bc9077 ("drm/vmwgfx: Add universal plane support")
Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v4.12+
---
drivers/gpu/drm/vmwgfx/vmwgfx_kms.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
index bf9931e3a728..bf24f2f0dcfc 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
@@ -233,10 +233,10 @@ struct vmw_framebuffer_bo {
static const uint32_t __maybe_unused vmw_primary_plane_formats[] = {
- DRM_FORMAT_XRGB1555,
- DRM_FORMAT_RGB565,
DRM_FORMAT_XRGB8888,
DRM_FORMAT_ARGB8888,
+ DRM_FORMAT_RGB565,
+ DRM_FORMAT_XRGB1555,
};
static const uint32_t __maybe_unused vmw_cursor_plane_formats[] = {
--
2.40.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 5/5] drm/vmwgfx: Sort primary plane formats by order of preference
2024-04-02 23:28 ` [PATCH 5/5] drm/vmwgfx: Sort primary plane formats by order of preference Zack Rusin
@ 2024-04-03 7:42 ` Pekka Paalanen
2024-04-03 11:44 ` Zack Rusin
0 siblings, 1 reply; 16+ messages in thread
From: Pekka Paalanen @ 2024-04-03 7:42 UTC (permalink / raw)
To: igt-dev
Cc: Zack Rusin, dri-devel, Broadcom internal kernel review list,
ian.forbes, martin.krastev, maaz.mombasawala, stable
[-- Attachment #1: Type: text/plain, Size: 1690 bytes --]
On Tue, 2 Apr 2024 19:28:13 -0400
Zack Rusin <zack.rusin@broadcom.com> wrote:
> The table of primary plane formats wasn't sorted at all, leading to
> applications picking our least desirable formats by defaults.
>
> Sort the primary plane formats according to our order of preference.
This is good.
> Fixes IGT's kms_atomic plane-invalid-params which assumes that the
> preferred format is a 32bpp format.
That sounds strange, why would IGT depend on preferred format being
32bpp?
That must be an oversight. IGT cannot dictate the format that hardware
must prefer. XRGB8888 is strongly suggested to be supported in general,
but why also preferred?
Thanks,
pq
> Signed-off-by: Zack Rusin <zack.rusin@broadcom.com>
> Fixes: 36cc79bc9077 ("drm/vmwgfx: Add universal plane support")
> Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v4.12+
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
> index bf9931e3a728..bf24f2f0dcfc 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
> @@ -233,10 +233,10 @@ struct vmw_framebuffer_bo {
>
>
> static const uint32_t __maybe_unused vmw_primary_plane_formats[] = {
> - DRM_FORMAT_XRGB1555,
> - DRM_FORMAT_RGB565,
> DRM_FORMAT_XRGB8888,
> DRM_FORMAT_ARGB8888,
> + DRM_FORMAT_RGB565,
> + DRM_FORMAT_XRGB1555,
> };
>
> static const uint32_t __maybe_unused vmw_cursor_plane_formats[] = {
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/5] drm/vmwgfx: Implement virtual crc generation
2024-04-02 23:28 ` [PATCH 2/5] drm/vmwgfx: Implement virtual crc generation Zack Rusin
@ 2024-04-03 11:31 ` kernel test robot
2024-04-09 15:08 ` Martin Krastev
1 sibling, 0 replies; 16+ messages in thread
From: kernel test robot @ 2024-04-03 11:31 UTC (permalink / raw)
To: Zack Rusin, dri-devel
Cc: llvm, oe-kbuild-all, Broadcom internal kernel review list,
ian.forbes, martin.krastev, maaz.mombasawala, Zack Rusin
Hi Zack,
kernel test robot noticed the following build warnings:
[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on drm/drm-next drm-exynos/exynos-drm-next drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-tip/drm-tip linus/master v6.9-rc2 next-20240403]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Zack-Rusin/drm-vmwgfx-Implement-virtual-kms/20240403-073132
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link: https://lore.kernel.org/r/20240402232813.2670131-3-zack.rusin%40broadcom.com
patch subject: [PATCH 2/5] drm/vmwgfx: Implement virtual crc generation
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20240403/202404031944.HMjSiHkr-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240403/202404031944.HMjSiHkr-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202404031944.HMjSiHkr-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c:515: warning: expecting prototype for vmw_vkms_lock_max_delay(). Prototype was for vmw_vkms_lock_max_wait_ns() instead
vim +515 drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
504
505 /**
506 * vmw_vkms_lock_max_delay - Return the max wait for the vkms lock
507 * @du: The vmw_display_unit from which to grab the vblank timings
508 *
509 * Returns the maximum wait time used to acquire the vkms lock. By
510 * default uses a time of a single frame and in case where vblank
511 * was not initialized for the display unit 1/60th of a second.
512 */
513 static inline u64
514 vmw_vkms_lock_max_wait_ns(struct vmw_display_unit *du)
> 515 {
516 s64 nsecs = ktime_to_ns(du->vkms.period_ns);
517
518 return (nsecs > 0) ? nsecs : 16666666;
519 }
520
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 5/5] drm/vmwgfx: Sort primary plane formats by order of preference
2024-04-03 7:42 ` Pekka Paalanen
@ 2024-04-03 11:44 ` Zack Rusin
2024-04-04 8:18 ` Pekka Paalanen
0 siblings, 1 reply; 16+ messages in thread
From: Zack Rusin @ 2024-04-03 11:44 UTC (permalink / raw)
To: Pekka Paalanen
Cc: igt-dev, dri-devel, Broadcom internal kernel review list,
ian.forbes, martin.krastev, maaz.mombasawala, stable
On Wed, Apr 3, 2024 at 3:43 AM Pekka Paalanen
<pekka.paalanen@collabora.com> wrote:
>
> On Tue, 2 Apr 2024 19:28:13 -0400
> Zack Rusin <zack.rusin@broadcom.com> wrote:
>
> > The table of primary plane formats wasn't sorted at all, leading to
> > applications picking our least desirable formats by defaults.
> >
> > Sort the primary plane formats according to our order of preference.
>
> This is good.
>
> > Fixes IGT's kms_atomic plane-invalid-params which assumes that the
> > preferred format is a 32bpp format.
>
> That sounds strange, why would IGT depend on preferred format being
> 32bpp?
>
> That must be an oversight. IGT cannot dictate the format that hardware
> must prefer. XRGB8888 is strongly suggested to be supported in general,
> but why also preferred?
I think it's just a side-effect of the pixman's assert that's failing:
https://cgit.freedesktop.org/drm/igt-gpu-tools/tree/lib/igt_fb.c#n4190
i.e. pixman assumes everything is 4 byte aligned.
I should have rephrased the message as "IGT assumes that the preferred
fb format is 4 byte aligned because our 16bpp formats are packed and
pixman can't convert them".
z
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 5/5] drm/vmwgfx: Sort primary plane formats by order of preference
2024-04-03 11:44 ` Zack Rusin
@ 2024-04-04 8:18 ` Pekka Paalanen
0 siblings, 0 replies; 16+ messages in thread
From: Pekka Paalanen @ 2024-04-04 8:18 UTC (permalink / raw)
To: Zack Rusin
Cc: igt-dev, dri-devel, Broadcom internal kernel review list,
ian.forbes, martin.krastev, maaz.mombasawala, stable
[-- Attachment #1: Type: text/plain, Size: 2114 bytes --]
On Wed, 3 Apr 2024 07:44:54 -0400
Zack Rusin <zack.rusin@broadcom.com> wrote:
> On Wed, Apr 3, 2024 at 3:43 AM Pekka Paalanen
> <pekka.paalanen@collabora.com> wrote:
> >
> > On Tue, 2 Apr 2024 19:28:13 -0400
> > Zack Rusin <zack.rusin@broadcom.com> wrote:
> >
> > > The table of primary plane formats wasn't sorted at all, leading to
> > > applications picking our least desirable formats by defaults.
> > >
> > > Sort the primary plane formats according to our order of preference.
> >
> > This is good.
> >
> > > Fixes IGT's kms_atomic plane-invalid-params which assumes that the
> > > preferred format is a 32bpp format.
> >
> > That sounds strange, why would IGT depend on preferred format being
> > 32bpp?
> >
> > That must be an oversight. IGT cannot dictate the format that hardware
> > must prefer. XRGB8888 is strongly suggested to be supported in general,
> > but why also preferred?
>
> I think it's just a side-effect of the pixman's assert that's failing:
> https://cgit.freedesktop.org/drm/igt-gpu-tools/tree/lib/igt_fb.c#n4190
> i.e. pixman assumes everything is 4 byte aligned.
> I should have rephrased the message as "IGT assumes that the preferred
> fb format is 4 byte aligned because our 16bpp formats are packed and
> pixman can't convert them".
Ah, yes. IIRC Pixman indeed assumes 4-byte alignment for stride and row
start. It should work for 16bpp formats if the FB had even width +
padding in pixels.
I think this is just an indication that Pixman is ill-suited for IGT.
IGT should be able to generate and analyse images in any format any
kernel driver might support.
I've noticed IGT also using Cairo, which limits the possible pixel
formats so severely I'm actually puzzled how it can even be used there.
Anyway, this is not at all about your patch, which looks good and well
to me. Just the comment about adapting to IGT seemed odd. Maybe phrase
that more like a happy accident rather than another justification for
this patch?
This patch:
Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Thanks,
pq
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 4/5] drm/vmwgfx: Fix crtc's atomic check conditional
2024-04-02 23:28 ` [PATCH 4/5] drm/vmwgfx: Fix crtc's atomic check conditional Zack Rusin
@ 2024-04-05 19:02 ` Ian Forbes
2024-04-09 15:12 ` Martin Krastev
1 sibling, 0 replies; 16+ messages in thread
From: Ian Forbes @ 2024-04-05 19:02 UTC (permalink / raw)
To: Zack Rusin
Cc: dri-devel, Broadcom internal kernel review list, martin.krastev,
maaz.mombasawala, stable
Makes sense.
Reviewed-by: Ian Forbes <ian.forbes@broadcom.com>
On Tue, Apr 2, 2024 at 6:28 PM Zack Rusin <zack.rusin@broadcom.com> wrote:
>
> The conditional was supposed to prevent enabling of a crtc state
> without a set primary plane. Accidently it also prevented disabling
> crtc state with a set primary plane. Neither is correct.
>
> Fix the conditional and just driver-warn when a crtc state has been
> enabled without a primary plane which will help debug broken userspace.
>
> Fixes IGT's kms_atomic_interruptible and kms_atomic_transition tests.
>
> Signed-off-by: Zack Rusin <zack.rusin@broadcom.com>
> Fixes: 06ec41909e31 ("drm/vmwgfx: Add and connect CRTC helper functions")
> Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v4.12+
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index e33e5993d8fc..13b2820cae51 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -931,6 +931,7 @@ int vmw_du_cursor_plane_atomic_check(struct drm_plane *plane,
> int vmw_du_crtc_atomic_check(struct drm_crtc *crtc,
> struct drm_atomic_state *state)
> {
> + struct vmw_private *vmw = vmw_priv(crtc->dev);
> struct drm_crtc_state *new_state = drm_atomic_get_new_crtc_state(state,
> crtc);
> struct vmw_display_unit *du = vmw_crtc_to_du(new_state->crtc);
> @@ -938,9 +939,13 @@ int vmw_du_crtc_atomic_check(struct drm_crtc *crtc,
> bool has_primary = new_state->plane_mask &
> drm_plane_mask(crtc->primary);
>
> - /* We always want to have an active plane with an active CRTC */
> - if (has_primary != new_state->enable)
> - return -EINVAL;
> + /*
> + * This is fine in general, but broken userspace might expect
> + * some actual rendering so give a clue as why it's blank.
> + */
> + if (new_state->enable && !has_primary)
> + drm_dbg_driver(&vmw->drm,
> + "CRTC without a primary plane will be blank.\n");
>
>
> if (new_state->connector_mask != connector_mask &&
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/5] drm/vmwgfx: Implement virtual kms
2024-04-02 23:28 ` [PATCH 1/5] drm/vmwgfx: Implement virtual kms Zack Rusin
@ 2024-04-05 21:53 ` Maaz Mombasawala
2024-04-05 22:26 ` Zack Rusin
0 siblings, 1 reply; 16+ messages in thread
From: Maaz Mombasawala @ 2024-04-05 21:53 UTC (permalink / raw)
To: Zack Rusin, dri-devel
Cc: Broadcom internal kernel review list, ian.forbes, martin.krastev
On 4/2/24 16:28, Zack Rusin wrote:
>
> @@ -541,6 +518,8 @@ static int vmw_ldu_init(struct vmw_private *dev_priv, unsigned unit)
> dev_priv->implicit_placement_property,
> 1);
>
> + vmw_du_init(&ldu->base);
> +
> return 0;
>
> err_free_unregister:
> @@ -905,6 +900,9 @@ static int vmw_sou_init(struct vmw_private *dev_priv, unsigned unit)
> dev->mode_config.suggested_x_property, 0);
> drm_object_attach_property(&connector->base,
> dev->mode_config.suggested_y_property, 0);
> +
> + vmw_du_init(&sou->base);
> +
> return 0;
>
> err_free_unregister:
> @@ -1575,6 +1576,9 @@ static int vmw_stdu_init(struct vmw_private *dev_priv, unsigned unit)
> dev->mode_config.suggested_x_property, 0);
> drm_object_attach_property(&connector->base,
> dev->mode_config.suggested_y_property, 0);
> +
> + vmw_du_init(&stdu->base);
> +
> return 0;
>
> err_free_unregister:
Shouldn't calls to vmw_du_init() be behind an if(vkms_enabled) condition?
Thanks,
Maaz Mombasawala <maaz.mombasawala@broadcom.com>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/5] drm/vmwgfx: Implement virtual kms
2024-04-05 21:53 ` Maaz Mombasawala
@ 2024-04-05 22:26 ` Zack Rusin
0 siblings, 0 replies; 16+ messages in thread
From: Zack Rusin @ 2024-04-05 22:26 UTC (permalink / raw)
To: Maaz Mombasawala
Cc: dri-devel, Broadcom internal kernel review list, ian.forbes,
martin.krastev
On Fri, Apr 5, 2024 at 5:53 PM Maaz Mombasawala
<maaz.mombasawala@broadcom.com> wrote:
>
> On 4/2/24 16:28, Zack Rusin wrote:
> >
> > @@ -541,6 +518,8 @@ static int vmw_ldu_init(struct vmw_private *dev_priv, unsigned unit)
> > dev_priv->implicit_placement_property,
> > 1);
> >
> > + vmw_du_init(&ldu->base);
> > +
> > return 0;
> >
> > err_free_unregister:
>
> > @@ -905,6 +900,9 @@ static int vmw_sou_init(struct vmw_private *dev_priv, unsigned unit)
> > dev->mode_config.suggested_x_property, 0);
> > drm_object_attach_property(&connector->base,
> > dev->mode_config.suggested_y_property, 0);
> > +
> > + vmw_du_init(&sou->base);
> > +
> > return 0;
> >
> > err_free_unregister:
>
> > @@ -1575,6 +1576,9 @@ static int vmw_stdu_init(struct vmw_private *dev_priv, unsigned unit)
> > dev->mode_config.suggested_x_property, 0);
> > drm_object_attach_property(&connector->base,
> > dev->mode_config.suggested_y_property, 0);
> > +
> > + vmw_du_init(&stdu->base);
> > +
> > return 0;
> >
> > err_free_unregister:
>
> Shouldn't calls to vmw_du_init() be behind an if(vkms_enabled) condition?
So the vmw_du_init is supposed to initialize the base, so that's
unconditional. To match the unconditional vmw_du_cleanup. There's an
argument to be made whether both of those should unconditionally call
vmw_vkms_crtc_init and vmw_vkms_crtc_cleanup. My opinion was that
they're not doing anything costly and just initialize members and
having the members of vmw_display_unit initialized whether vkms is
enabled or not still makes sense.
z
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/5] drm/vmwgfx: Implement virtual crc generation
2024-04-02 23:28 ` [PATCH 2/5] drm/vmwgfx: Implement virtual crc generation Zack Rusin
2024-04-03 11:31 ` kernel test robot
@ 2024-04-09 15:08 ` Martin Krastev
1 sibling, 0 replies; 16+ messages in thread
From: Martin Krastev @ 2024-04-09 15:08 UTC (permalink / raw)
To: Zack Rusin
Cc: dri-devel, Broadcom internal kernel review list, ian.forbes,
maaz.mombasawala
On Wed, Apr 3, 2024 at 2:28 AM Zack Rusin <zack.rusin@broadcom.com> wrote:
>
> crc checksums are used to validate the output. Normally they're part
> of the actual display hardware but on virtual stack there's nothing
> to automatically generate them.
>
> Implement crc generation for the vmwgfx stack. This works only on
> screen targets, where it's possibly to easily make sure that the
> guest side contents of the surface matches the host sides output.
>
> Just like the vblank support, crc generation can only be enabled via:
> guestinfo.vmwgfx.vkms_enable = "TRUE"
> option in the vmx file.
>
> Makes IGT's kms_pipe_crc_basic pass and allows a huge number of other
> IGT tests which require CRC generation of the output to actually run
> on vmwgfx. Makes it possible to actually validate a lof of the kms and
> drm functionality with vmwgfx.
>
> Signed-off-by: Zack Rusin <zack.rusin@broadcom.com>
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 1 +
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 2 +
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 31 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.h | 15 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 32 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 22 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c | 453 ++++++++++++++++++++++-
> drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h | 28 +-
> 8 files changed, 550 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index e34c48fd25d4..89d3679d2608 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -1198,6 +1198,7 @@ static void vmw_driver_unload(struct drm_device *dev)
>
> vmw_svga_disable(dev_priv);
>
> + vmw_vkms_cleanup(dev_priv);
> vmw_kms_close(dev_priv);
> vmw_overlay_close(dev_priv);
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> index 4f5d7d13c4aa..ddbceaa31b59 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> @@ -616,6 +616,7 @@ struct vmw_private {
> uint32 *devcaps;
>
> bool vkms_enabled;
> + struct workqueue_struct *crc_workq;
>
> /*
> * mksGuestStat instance-descriptor and pid arrays
> @@ -811,6 +812,7 @@ void vmw_resource_mob_attach(struct vmw_resource *res);
> void vmw_resource_mob_detach(struct vmw_resource *res);
> void vmw_resource_dirty_update(struct vmw_resource *res, pgoff_t start,
> pgoff_t end);
> +int vmw_resource_clean(struct vmw_resource *res);
> int vmw_resources_clean(struct vmw_bo *vbo, pgoff_t start,
> pgoff_t end, pgoff_t *num_prefault);
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index e763cf0e6cfc..e33e5993d8fc 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -40,14 +40,14 @@
>
> void vmw_du_init(struct vmw_display_unit *du)
> {
> - hrtimer_init(&du->vkms.timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> - du->vkms.timer.function = &vmw_vkms_vblank_simulate;
> + vmw_vkms_crtc_init(&du->crtc);
> }
>
> void vmw_du_cleanup(struct vmw_display_unit *du)
> {
> struct vmw_private *dev_priv = vmw_priv(du->primary.dev);
> - hrtimer_cancel(&du->vkms.timer);
> +
> + vmw_vkms_crtc_cleanup(&du->crtc);
> drm_plane_cleanup(&du->primary);
> if (vmw_cmd_supported(dev_priv))
> drm_plane_cleanup(&du->cursor.base);
> @@ -963,6 +963,7 @@ int vmw_du_crtc_atomic_check(struct drm_crtc *crtc,
> void vmw_du_crtc_atomic_begin(struct drm_crtc *crtc,
> struct drm_atomic_state *state)
> {
> + vmw_vkms_crtc_atomic_begin(crtc, state);
> }
>
> /**
> @@ -2029,6 +2030,29 @@ vmw_kms_create_hotplug_mode_update_property(struct vmw_private *dev_priv)
> "hotplug_mode_update", 0, 1);
> }
>
> +static void
> +vmw_atomic_commit_tail(struct drm_atomic_state *old_state)
> +{
> + struct vmw_private *vmw = vmw_priv(old_state->dev);
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *old_crtc_state;
> + int i;
> +
> + drm_atomic_helper_commit_tail(old_state);
> +
> + if (vmw->vkms_enabled) {
> + for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) {
> + struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
> + (void)old_crtc_state;
> + flush_work(&du->vkms.crc_generator_work);
> + }
> + }
> +}
> +
> +static const struct drm_mode_config_helper_funcs vmw_mode_config_helpers = {
> + .atomic_commit_tail = vmw_atomic_commit_tail,
> +};
> +
> int vmw_kms_init(struct vmw_private *dev_priv)
> {
> struct drm_device *dev = &dev_priv->drm;
> @@ -2048,6 +2072,7 @@ int vmw_kms_init(struct vmw_private *dev_priv)
> dev->mode_config.max_width = dev_priv->texture_max_width;
> dev->mode_config.max_height = dev_priv->texture_max_height;
> dev->mode_config.preferred_depth = dev_priv->assume_16bpp ? 16 : 32;
> + dev->mode_config.helper_private = &vmw_mode_config_helpers;
>
> drm_mode_create_suggested_offset_properties(dev);
> vmw_kms_create_hotplug_mode_update_property(dev_priv);
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
> index 9e83a1553286..bf9931e3a728 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
> @@ -378,9 +378,22 @@ struct vmw_display_unit {
> int set_gui_y;
>
> struct {
> + struct work_struct crc_generator_work;
> struct hrtimer timer;
> ktime_t period_ns;
> - struct drm_pending_vblank_event *event;
> +
> + /* protects concurrent access to the vblank handler */
> + atomic_t atomic_lock;
> + /* protected by @atomic_lock */
> + bool crc_enabled;
> + struct vmw_surface *surface;
> +
> + /* protects concurrent access to the crc worker */
> + spinlock_t crc_state_lock;
> + /* protected by @crc_state_lock */
> + bool crc_pending;
> + u64 frame_start;
> + u64 frame_end;
> } vkms;
> };
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> index ca300c7427d2..848dba09981b 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> @@ -1064,6 +1064,22 @@ void vmw_resource_dirty_update(struct vmw_resource *res, pgoff_t start,
> end << PAGE_SHIFT);
> }
>
> +int vmw_resource_clean(struct vmw_resource *res)
> +{
> + int ret = 0;
> +
> + if (res->res_dirty) {
> + if (!res->func->clean)
> + return -EINVAL;
> +
> + ret = res->func->clean(res);
> + if (ret)
> + return ret;
> + res->res_dirty = false;
> + }
> + return ret;
> +}
> +
> /**
> * vmw_resources_clean - Clean resources intersecting a mob range
> * @vbo: The mob buffer object
> @@ -1080,6 +1096,7 @@ int vmw_resources_clean(struct vmw_bo *vbo, pgoff_t start,
> unsigned long res_start = start << PAGE_SHIFT;
> unsigned long res_end = end << PAGE_SHIFT;
> unsigned long last_cleaned = 0;
> + int ret;
>
> /*
> * Find the resource with lowest backup_offset that intersects the
> @@ -1106,18 +1123,9 @@ int vmw_resources_clean(struct vmw_bo *vbo, pgoff_t start,
> * intersecting the range.
> */
> while (found) {
> - if (found->res_dirty) {
> - int ret;
> -
> - if (!found->func->clean)
> - return -EINVAL;
> -
> - ret = found->func->clean(found);
> - if (ret)
> - return ret;
> -
> - found->res_dirty = false;
> - }
> + ret = vmw_resource_clean(found);
> + if (ret)
> + return ret;
> last_cleaned = found->guest_memory_offset + found->guest_memory_size;
> cur = rb_next(&found->mob_node);
> if (!cur)
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> index 665bde7e0be0..2041c4d48daa 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> @@ -409,11 +409,6 @@ static void vmw_stdu_crtc_mode_set_nofb(struct drm_crtc *crtc)
> crtc->x, crtc->y);
> }
>
> -
> -static void vmw_stdu_crtc_helper_prepare(struct drm_crtc *crtc)
> -{
> -}
> -
> static void vmw_stdu_crtc_atomic_disable(struct drm_crtc *crtc,
> struct drm_atomic_state *state)
> {
> @@ -783,6 +778,9 @@ static const struct drm_crtc_funcs vmw_stdu_crtc_funcs = {
> .enable_vblank = vmw_vkms_enable_vblank,
> .disable_vblank = vmw_vkms_disable_vblank,
> .get_vblank_timestamp = vmw_vkms_get_vblank_timestamp,
> + .get_crc_sources = vmw_vkms_get_crc_sources,
> + .set_crc_source = vmw_vkms_set_crc_source,
> + .verify_crc_source = vmw_vkms_verify_crc_source,
> };
>
>
> @@ -1414,6 +1412,17 @@ vmw_stdu_primary_plane_atomic_update(struct drm_plane *plane,
> vmw_fence_obj_unreference(&fence);
> }
>
> +static void
> +vmw_stdu_crtc_atomic_flush(struct drm_crtc *crtc,
> + struct drm_atomic_state *state)
> +{
> + struct vmw_private *vmw = vmw_priv(crtc->dev);
> + struct vmw_screen_target_display_unit *stdu = vmw_crtc_to_stdu(crtc);
> +
> + if (vmw->vkms_enabled)
> + vmw_vkms_set_crc_surface(crtc, stdu->display_srf);
> + vmw_vkms_crtc_atomic_flush(crtc, state);
> +}
>
> static const struct drm_plane_funcs vmw_stdu_plane_funcs = {
> .update_plane = drm_atomic_helper_update_plane,
> @@ -1454,11 +1463,10 @@ drm_plane_helper_funcs vmw_stdu_primary_plane_helper_funcs = {
> };
>
> static const struct drm_crtc_helper_funcs vmw_stdu_crtc_helper_funcs = {
> - .prepare = vmw_stdu_crtc_helper_prepare,
> .mode_set_nofb = vmw_stdu_crtc_mode_set_nofb,
> .atomic_check = vmw_du_crtc_atomic_check,
> .atomic_begin = vmw_du_crtc_atomic_begin,
> - .atomic_flush = vmw_vkms_crtc_atomic_flush,
> + .atomic_flush = vmw_stdu_crtc_atomic_flush,
> .atomic_enable = vmw_vkms_crtc_atomic_enable,
> .atomic_disable = vmw_stdu_crtc_atomic_disable,
> };
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
> index ff76bfd70f91..5138a7107897 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c
> @@ -28,33 +28,179 @@
>
> #include "vmwgfx_vkms.h"
>
> +#include "vmwgfx_bo.h"
> #include "vmwgfx_drv.h"
> #include "vmwgfx_kms.h"
> #include "vmwgfx_vkms.h"
>
> +#include "vmw_surface_cache.h"
> +
> #include <drm/drm_crtc.h>
> +#include <drm/drm_debugfs_crc.h>
> #include <drm/drm_print.h>
> #include <drm/drm_vblank.h>
>
> +#include <linux/crc32.h>
> +#include <linux/delay.h>
> +
> #define GUESTINFO_VBLANK "guestinfo.vmwgfx.vkms_enable"
>
> -enum hrtimer_restart
> +static int
> +vmw_surface_sync(struct vmw_private *vmw,
> + struct vmw_surface *surf)
> +{
> + int ret;
> + struct vmw_fence_obj *fence = NULL;
> + struct vmw_bo *bo = surf->res.guest_memory_bo;
> +
> + vmw_resource_clean(&surf->res);
> +
> + ret = ttm_bo_reserve(&bo->tbo, false, false, NULL);
> + if (ret != 0) {
> + drm_warn(&vmw->drm, "%s: failed reserve\n", __func__);
> + goto done;
> + }
> +
> + ret = vmw_execbuf_fence_commands(NULL, vmw, &fence, NULL);
> + if (ret != 0) {
> + drm_warn(&vmw->drm, "%s: failed execbuf\n", __func__);
> + ttm_bo_unreserve(&bo->tbo);
> + goto done;
> + }
> +
> + dma_fence_wait(&fence->base, false);
> + dma_fence_put(&fence->base);
> +
> + ttm_bo_unreserve(&bo->tbo);
> +done:
> + return ret;
> +}
> +
> +static int
> +compute_crc(struct drm_crtc *crtc,
> + struct vmw_surface *surf,
> + u32 *crc)
> +{
> + u8 *mapped_surface;
> + struct vmw_bo *bo = surf->res.guest_memory_bo;
> + const struct SVGA3dSurfaceDesc *desc =
> + vmw_surface_get_desc(surf->metadata.format);
> + u32 row_pitch_bytes;
> + SVGA3dSize blocks;
> + u32 y;
> +
> + *crc = 0;
> +
> + vmw_surface_get_size_in_blocks(desc, &surf->metadata.base_size, &blocks);
> + row_pitch_bytes = blocks.width * desc->pitchBytesPerBlock;
> + WARN_ON(!bo);
> + mapped_surface = vmw_bo_map_and_cache(bo);
> +
> + for (y = 0; y < blocks.height; y++) {
> + *crc = crc32_le(*crc, mapped_surface, row_pitch_bytes);
> + mapped_surface += row_pitch_bytes;
> + }
> +
> + vmw_bo_unmap(bo);
> +
> + return 0;
> +}
> +
> +static void
> +crc_generate_worker(struct work_struct *work)
> +{
> + struct vmw_display_unit *du =
> + container_of(work, struct vmw_display_unit, vkms.crc_generator_work);
> + struct drm_crtc *crtc = &du->crtc;
> + struct vmw_private *vmw = vmw_priv(crtc->dev);
> + bool crc_pending;
> + u64 frame_start, frame_end;
> + u32 crc32 = 0;
> + struct vmw_surface *surf = 0;
> + int ret;
> +
> + spin_lock_irq(&du->vkms.crc_state_lock);
> + crc_pending = du->vkms.crc_pending;
> + spin_unlock_irq(&du->vkms.crc_state_lock);
> +
> + /*
> + * We raced with the vblank hrtimer and previous work already computed
> + * the crc, nothing to do.
> + */
> + if (!crc_pending)
> + return;
> +
> + spin_lock_irq(&du->vkms.crc_state_lock);
> + surf = du->vkms.surface;
> + spin_unlock_irq(&du->vkms.crc_state_lock);
> +
> + if (vmw_surface_sync(vmw, surf)) {
> + drm_warn(crtc->dev, "CRC worker wasn't able to sync the crc surface!\n");
> + return;
> + }
> +
> + ret = compute_crc(crtc, surf, &crc32);
> + if (ret)
> + return;
> +
> + spin_lock_irq(&du->vkms.crc_state_lock);
> + frame_start = du->vkms.frame_start;
> + frame_end = du->vkms.frame_end;
> + crc_pending = du->vkms.crc_pending;
> + du->vkms.frame_start = 0;
> + du->vkms.frame_end = 0;
> + du->vkms.crc_pending = false;
> + spin_unlock_irq(&du->vkms.crc_state_lock);
> +
> + /*
> + * The worker can fall behind the vblank hrtimer, make sure we catch up.
> + */
> + while (frame_start <= frame_end)
> + drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);
> +}
> +
> +static enum hrtimer_restart
> vmw_vkms_vblank_simulate(struct hrtimer *timer)
> {
> struct vmw_display_unit *du = container_of(timer, struct vmw_display_unit, vkms.timer);
> struct drm_crtc *crtc = &du->crtc;
> + struct vmw_private *vmw = vmw_priv(crtc->dev);
> + struct vmw_surface *surf = NULL;
> u64 ret_overrun;
> - bool ret;
> + bool locked, ret;
>
> ret_overrun = hrtimer_forward_now(&du->vkms.timer,
> du->vkms.period_ns);
> if (ret_overrun != 1)
> - DRM_WARN("%s: vblank timer overrun\n", __func__);
> + drm_dbg_driver(crtc->dev, "vblank timer missed %lld frames.\n",
> + ret_overrun - 1);
>
> + locked = vmw_vkms_vblank_trylock(crtc);
> ret = drm_crtc_handle_vblank(crtc);
> - /* Don't queue timer again when vblank is disabled. */
> - if (!ret)
> - return HRTIMER_NORESTART;
> + WARN_ON(!ret);
> + if (!locked)
> + return HRTIMER_RESTART;
> + surf = du->vkms.surface;
> + vmw_vkms_unlock(crtc);
> +
> + if (du->vkms.crc_enabled && surf) {
> + u64 frame = drm_crtc_accurate_vblank_count(crtc);
> +
> + spin_lock(&du->vkms.crc_state_lock);
> + if (!du->vkms.crc_pending)
> + du->vkms.frame_start = frame;
> + else
> + drm_dbg_driver(crtc->dev,
> + "crc worker falling behind, frame_start: %llu, frame_end: %llu\n",
> + du->vkms.frame_start, frame);
> + du->vkms.frame_end = frame;
> + du->vkms.crc_pending = true;
> + spin_unlock(&du->vkms.crc_state_lock);
> +
> + ret = queue_work(vmw->crc_workq, &du->vkms.crc_generator_work);
> + if (!ret)
> + drm_dbg_driver(crtc->dev, "Composer worker already queued\n");
> + }
>
> return HRTIMER_RESTART;
> }
> @@ -78,8 +224,21 @@ vmw_vkms_init(struct vmw_private *vmw)
> if (!ret && vmw->vkms_enabled) {
> ret = drm_vblank_init(&vmw->drm, VMWGFX_NUM_DISPLAY_UNITS);
> vmw->vkms_enabled = (ret == 0);
> - drm_info(&vmw->drm, "vkms_enabled = %d\n", vmw->vkms_enabled);
> }
> +
> + vmw->crc_workq = alloc_ordered_workqueue("vmwgfx_crc_generator", 0);
> + if (!vmw->crc_workq) {
> + drm_warn(&vmw->drm, "crc workqueue allocation failed. Disabling vkms.");
> + vmw->vkms_enabled = false;
> + }
> + if (vmw->vkms_enabled)
> + drm_info(&vmw->drm, "VKMS enabled\n");
> +}
> +
> +void
> +vmw_vkms_cleanup(struct vmw_private *vmw)
> +{
> + destroy_workqueue(vmw->crc_workq);
> }
>
> bool
> @@ -133,6 +292,8 @@ vmw_vkms_enable_vblank(struct drm_crtc *crtc)
>
> drm_calc_timestamping_constants(crtc, &crtc->mode);
>
> + hrtimer_init(&du->vkms.timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> + du->vkms.timer.function = &vmw_vkms_vblank_simulate;
> du->vkms.period_ns = ktime_set(0, vblank->framedur_ns);
> hrtimer_start(&du->vkms.timer, du->vkms.period_ns, HRTIMER_MODE_REL);
>
> @@ -148,7 +309,46 @@ vmw_vkms_disable_vblank(struct drm_crtc *crtc)
> if (!vmw->vkms_enabled)
> return;
>
> - hrtimer_try_to_cancel(&du->vkms.timer);
> + hrtimer_cancel(&du->vkms.timer);
> + du->vkms.surface = NULL;
> + du->vkms.period_ns = ktime_set(0, 0);
> +}
> +
> +enum vmw_vkms_lock_state {
> + VMW_VKMS_LOCK_UNLOCKED = 0,
> + VMW_VKMS_LOCK_MODESET = 1,
> + VMW_VKMS_LOCK_VBLANK = 2
> +};
> +
> +void
> +vmw_vkms_crtc_init(struct drm_crtc *crtc)
> +{
> + struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
> +
> + atomic_set(&du->vkms.atomic_lock, VMW_VKMS_LOCK_UNLOCKED);
> + spin_lock_init(&du->vkms.crc_state_lock);
> +
> + INIT_WORK(&du->vkms.crc_generator_work, crc_generate_worker);
> + du->vkms.surface = NULL;
> +}
> +
> +void
> +vmw_vkms_crtc_cleanup(struct drm_crtc *crtc)
> +{
> + struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
> +
> + WARN_ON(work_pending(&du->vkms.crc_generator_work));
> + hrtimer_cancel(&du->vkms.timer);
> +}
> +
> +void
> +vmw_vkms_crtc_atomic_begin(struct drm_crtc *crtc,
> + struct drm_atomic_state *state)
> +{
> + struct vmw_private *vmw = vmw_priv(crtc->dev);
> +
> + if (vmw->vkms_enabled)
> + vmw_vkms_modeset_lock(crtc);
> }
>
> void
> @@ -170,6 +370,9 @@ vmw_vkms_crtc_atomic_flush(struct drm_crtc *crtc,
>
> crtc->state->event = NULL;
> }
> +
> + if (vmw->vkms_enabled)
> + vmw_vkms_unlock(crtc);
> }
An efficiency/codegen-related nitpick that spans two of the changes in this set:
The predicate here -- vmw->vmks_enabled, is also used in the previous
if-then statement as:
if (vmw->vkms_enabled && crtc->state->event) {
...
}
if (vmw->vkms_enabled)
vmw_vkms_unlock(crtc);
Basically, all work in vmw_vkms_crtc_atomic_flush() is conditioned on
that particular predicate, so we can just as well early-out on
vkms_enabled low. Alternatively, if early-out is to be avoided, we can
fetch the predicate just once, as we don't expect it to change for the
duration of the function.
>
> void
> @@ -191,3 +394,237 @@ vmw_vkms_crtc_atomic_disable(struct drm_crtc *crtc,
> if (vmw->vkms_enabled)
> drm_crtc_vblank_off(crtc);
> }
> +
> +static bool
> +is_crc_supported(struct drm_crtc *crtc)
> +{
> + struct vmw_private *vmw = vmw_priv(crtc->dev);
> +
> + if (!vmw->vkms_enabled)
> + return false;
> +
> + if (vmw->active_display_unit != vmw_du_screen_target)
> + return false;
> +
> + return true;
> +}
> +
> +static const char * const pipe_crc_sources[] = {"auto"};
> +
> +static int
> +crc_parse_source(const char *src_name,
> + bool *enabled)
> +{
> + int ret = 0;
> +
> + if (!src_name) {
> + *enabled = false;
> + } else if (strcmp(src_name, "auto") == 0) {
> + *enabled = true;
> + } else {
> + *enabled = false;
> + ret = -EINVAL;
> + }
> +
> + return ret;
> +}
> +
> +const char *const *
> +vmw_vkms_get_crc_sources(struct drm_crtc *crtc,
> + size_t *count)
> +{
> + *count = 0;
> + if (!is_crc_supported(crtc))
> + return NULL;
> +
> + *count = ARRAY_SIZE(pipe_crc_sources);
> + return pipe_crc_sources;
> +}
> +
> +int
> +vmw_vkms_verify_crc_source(struct drm_crtc *crtc,
> + const char *src_name,
> + size_t *values_cnt)
> +{
> + bool enabled;
> +
> + if (!is_crc_supported(crtc))
> + return -EINVAL;
> +
> + if (crc_parse_source(src_name, &enabled) < 0) {
> + drm_dbg_driver(crtc->dev, "unknown source '%s'\n", src_name);
> + return -EINVAL;
> + }
> +
> + *values_cnt = 1;
> +
> + return 0;
> +}
> +
> +int
> +vmw_vkms_set_crc_source(struct drm_crtc *crtc,
> + const char *src_name)
> +{
> + struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
> + bool enabled, prev_enabled, locked;
> + int ret;
> +
> + if (!is_crc_supported(crtc))
> + return -EINVAL;
> +
> + ret = crc_parse_source(src_name, &enabled);
> +
> + if (enabled)
> + drm_crtc_vblank_get(crtc);
> +
> + locked = vmw_vkms_modeset_lock_relaxed(crtc);
> + prev_enabled = du->vkms.crc_enabled;
> + du->vkms.crc_enabled = enabled;
> + if (locked)
> + vmw_vkms_unlock(crtc);
> +
> + if (prev_enabled)
> + drm_crtc_vblank_put(crtc);
> +
> + return ret;
> +}
> +
> +void
> +vmw_vkms_set_crc_surface(struct drm_crtc *crtc,
> + struct vmw_surface *surf)
> +{
> + struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
> + struct vmw_private *vmw = vmw_priv(crtc->dev);
> +
> + if (vmw->vkms_enabled) {
> + WARN_ON(atomic_read(&du->vkms.atomic_lock) != VMW_VKMS_LOCK_MODESET);
> + du->vkms.surface = surf;
> + }
> +}
> +
> +/**
> + * vmw_vkms_lock_max_delay - Return the max wait for the vkms lock
> + * @du: The vmw_display_unit from which to grab the vblank timings
> + *
> + * Returns the maximum wait time used to acquire the vkms lock. By
> + * default uses a time of a single frame and in case where vblank
> + * was not initialized for the display unit 1/60th of a second.
> + */
> +static inline u64
> +vmw_vkms_lock_max_wait_ns(struct vmw_display_unit *du)
> +{
> + s64 nsecs = ktime_to_ns(du->vkms.period_ns);
> +
> + return (nsecs > 0) ? nsecs : 16666666;
> +}
> +
> +/**
> + * vmw_vkms_modeset_lock - Protects access to crtc during modeset
> + * @crtc: The crtc to lock for vkms
> + *
> + * This function prevents the VKMS timers/callbacks from being called
> + * while a modeset operation is in process. We don't want the callbacks
> + * e.g. the vblank simulator to be trying to access incomplete state
> + * so we need to make sure they execute only when the modeset has
> + * finished.
> + *
> + * Normally this would have been done with a spinlock but locking the
> + * entire atomic modeset with vmwgfx is impossible because kms prepare
> + * executes non-atomic ops (e.g. vmw_validation_prepare holds a mutex to
> + * guard various bits of state). Which means that we need to synchronize
> + * atomic context (the vblank handler) with the non-atomic entirity
> + * of kms - so use an atomic_t to track which part of vkms has access
> + * to the basic vkms state.
> + */
> +void
> +vmw_vkms_modeset_lock(struct drm_crtc *crtc)
> +{
> + struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
> + const u64 nsecs_delay = 10;
> + const u64 MAX_NSECS_DELAY = vmw_vkms_lock_max_wait_ns(du);
> + u64 total_delay = 0;
> + int ret;
> +
> + do {
> + ret = atomic_cmpxchg(&du->vkms.atomic_lock,
> + VMW_VKMS_LOCK_UNLOCKED,
> + VMW_VKMS_LOCK_MODESET);
> + if (ret == VMW_VKMS_LOCK_UNLOCKED || total_delay >= MAX_NSECS_DELAY)
> + break;
> + ndelay(nsecs_delay);
> + total_delay += nsecs_delay;
> + } while (1);
> +
> + if (total_delay >= MAX_NSECS_DELAY) {
> + drm_warn(crtc->dev, "VKMS lock expired! total_delay = %lld, ret = %d, cur = %d\n",
> + total_delay, ret, atomic_read(&du->vkms.atomic_lock));
> + }
> +}
> +
> +/**
> + * vmw_vkms_modeset_lock_relaxed - Protects access to crtc during modeset
> + * @crtc: The crtc to lock for vkms
> + *
> + * Much like vmw_vkms_modeset_lock except that when the crtc is currently
> + * in a modeset it will return immediately.
> + *
> + * Returns true if actually locked vkms to modeset or false otherwise.
> + */
> +bool
> +vmw_vkms_modeset_lock_relaxed(struct drm_crtc *crtc)
> +{
> + struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
> + const u64 nsecs_delay = 10;
> + const u64 MAX_NSECS_DELAY = vmw_vkms_lock_max_wait_ns(du);
> + u64 total_delay = 0;
> + int ret;
> +
> + do {
> + ret = atomic_cmpxchg(&du->vkms.atomic_lock,
> + VMW_VKMS_LOCK_UNLOCKED,
> + VMW_VKMS_LOCK_MODESET);
> + if (ret == VMW_VKMS_LOCK_UNLOCKED ||
> + ret == VMW_VKMS_LOCK_MODESET ||
> + total_delay >= MAX_NSECS_DELAY)
> + break;
> + ndelay(nsecs_delay);
> + total_delay += nsecs_delay;
> + } while (1);
> +
> + if (total_delay >= MAX_NSECS_DELAY) {
> + drm_warn(crtc->dev, "VKMS relaxed lock expired!\n");
> + return false;
> + }
> +
> + return ret == VMW_VKMS_LOCK_UNLOCKED;
> +}
> +
> +/**
> + * vmw_vkms_vblank_trylock - Protects access to crtc during vblank
> + * @crtc: The crtc to lock for vkms
> + *
> + * Tries to lock vkms for vblank, returns immediately.
> + *
> + * Returns true if locked vkms to vblank or false otherwise.
> + */
> +bool
> +vmw_vkms_vblank_trylock(struct drm_crtc *crtc)
> +{
> + struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
> + u32 ret;
> +
> + ret = atomic_cmpxchg(&du->vkms.atomic_lock,
> + VMW_VKMS_LOCK_UNLOCKED,
> + VMW_VKMS_LOCK_VBLANK);
> +
> + return ret == VMW_VKMS_LOCK_UNLOCKED;
> +}
> +
> +void
> +vmw_vkms_unlock(struct drm_crtc *crtc)
> +{
> + struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
> +
> + /* Release flag; mark it as unlocked. */
> + atomic_set(&du->vkms.atomic_lock, VMW_VKMS_LOCK_UNLOCKED);
> +}
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h
> index 0f6d4ab9ebe3..69ddd33a8444 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.h
> @@ -32,22 +32,44 @@
> #include <linux/hrtimer_types.h>
> #include <linux/types.h>
>
> -struct vmw_private;
> -struct drm_crtc;
> struct drm_atomic_state;
> +struct drm_crtc;
> +struct vmw_private;
> +struct vmw_surface;
>
> void vmw_vkms_init(struct vmw_private *vmw);
> +void vmw_vkms_cleanup(struct vmw_private *vmw);
> +
> +void vmw_vkms_modeset_lock(struct drm_crtc *crtc);
> +bool vmw_vkms_modeset_lock_relaxed(struct drm_crtc *crtc);
> +bool vmw_vkms_vblank_trylock(struct drm_crtc *crtc);
> +void vmw_vkms_unlock(struct drm_crtc *crtc);
> +
> bool vmw_vkms_get_vblank_timestamp(struct drm_crtc *crtc,
> int *max_error,
> ktime_t *vblank_time,
> bool in_vblank_irq);
> int vmw_vkms_enable_vblank(struct drm_crtc *crtc);
> void vmw_vkms_disable_vblank(struct drm_crtc *crtc);
> +
> +void vmw_vkms_crtc_init(struct drm_crtc *crtc);
> +void vmw_vkms_crtc_cleanup(struct drm_crtc *crtc);
> +void vmw_vkms_crtc_atomic_begin(struct drm_crtc *crtc,
> + struct drm_atomic_state *state);
> void vmw_vkms_crtc_atomic_flush(struct drm_crtc *crtc, struct drm_atomic_state *state);
> -enum hrtimer_restart vmw_vkms_vblank_simulate(struct hrtimer *timer);
> void vmw_vkms_crtc_atomic_enable(struct drm_crtc *crtc,
> struct drm_atomic_state *state);
> void vmw_vkms_crtc_atomic_disable(struct drm_crtc *crtc,
> struct drm_atomic_state *state);
>
> +const char *const *vmw_vkms_get_crc_sources(struct drm_crtc *crtc,
> + size_t *count);
> +int vmw_vkms_verify_crc_source(struct drm_crtc *crtc,
> + const char *src_name,
> + size_t *values_cnt);
> +int vmw_vkms_set_crc_source(struct drm_crtc *crtc,
> + const char *src_name);
> +void vmw_vkms_set_crc_surface(struct drm_crtc *crtc,
> + struct vmw_surface *surf);
> +
> #endif
> --
> 2.40.1
>
Regards,
Martin
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 4/5] drm/vmwgfx: Fix crtc's atomic check conditional
2024-04-02 23:28 ` [PATCH 4/5] drm/vmwgfx: Fix crtc's atomic check conditional Zack Rusin
2024-04-05 19:02 ` Ian Forbes
@ 2024-04-09 15:12 ` Martin Krastev
1 sibling, 0 replies; 16+ messages in thread
From: Martin Krastev @ 2024-04-09 15:12 UTC (permalink / raw)
To: Zack Rusin
Cc: dri-devel, Broadcom internal kernel review list, ian.forbes,
maaz.mombasawala, stable
On Wed, Apr 3, 2024 at 2:28 AM Zack Rusin <zack.rusin@broadcom.com> wrote:
>
> The conditional was supposed to prevent enabling of a crtc state
> without a set primary plane. Accidently it also prevented disabling
> crtc state with a set primary plane. Neither is correct.
>
> Fix the conditional and just driver-warn when a crtc state has been
> enabled without a primary plane which will help debug broken userspace.
>
> Fixes IGT's kms_atomic_interruptible and kms_atomic_transition tests.
>
> Signed-off-by: Zack Rusin <zack.rusin@broadcom.com>
> Fixes: 06ec41909e31 ("drm/vmwgfx: Add and connect CRTC helper functions")
> Cc: Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v4.12+
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index e33e5993d8fc..13b2820cae51 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -931,6 +931,7 @@ int vmw_du_cursor_plane_atomic_check(struct drm_plane *plane,
> int vmw_du_crtc_atomic_check(struct drm_crtc *crtc,
> struct drm_atomic_state *state)
> {
> + struct vmw_private *vmw = vmw_priv(crtc->dev);
> struct drm_crtc_state *new_state = drm_atomic_get_new_crtc_state(state,
> crtc);
> struct vmw_display_unit *du = vmw_crtc_to_du(new_state->crtc);
> @@ -938,9 +939,13 @@ int vmw_du_crtc_atomic_check(struct drm_crtc *crtc,
> bool has_primary = new_state->plane_mask &
> drm_plane_mask(crtc->primary);
>
> - /* We always want to have an active plane with an active CRTC */
> - if (has_primary != new_state->enable)
> - return -EINVAL;
> + /*
> + * This is fine in general, but broken userspace might expect
> + * some actual rendering so give a clue as why it's blank.
> + */
> + if (new_state->enable && !has_primary)
> + drm_dbg_driver(&vmw->drm,
> + "CRTC without a primary plane will be blank.\n");
>
>
> if (new_state->connector_mask != connector_mask &&
> --
> 2.40.1
>
LGTM!
Reviewed-by: Martin Krastev <martin.krastev@broadcom.com>
Regards,
Martin
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/5] drm/vmwgfx: Fix prime import/export
2024-04-02 23:28 ` [PATCH 3/5] drm/vmwgfx: Fix prime import/export Zack Rusin
@ 2024-04-09 15:30 ` Martin Krastev
0 siblings, 0 replies; 16+ messages in thread
From: Martin Krastev @ 2024-04-09 15:30 UTC (permalink / raw)
To: Zack Rusin
Cc: dri-devel, Broadcom internal kernel review list, ian.forbes,
maaz.mombasawala, stable
Great to have this!
Reviewed-by: Martin Krastev <martin.krastev@broadcom.com>
Regards,
Martin
On Wed, Apr 3, 2024 at 2:28 AM Zack Rusin <zack.rusin@broadcom.com> wrote:
>
> vmwgfx never supported prime import of external buffers. Furthermore the
> driver exposes two different objects to userspace: vmw_surface's and
> gem buffers but prime import/export only worked with vmw_surfaces.
>
> Because gem buffers are used through the dumb_buffer interface this meant
> that the driver created buffers couldn't have been prime exported or
> imported.
>
> Fix prime import/export. Makes IGT's kms_prime pass.
>
> Signed-off-by: Zack Rusin <zack.rusin@broadcom.com>
> Fixes: 8afa13a0583f ("drm/vmwgfx: Implement DRIVER_GEM")
> Cc: <stable@vger.kernel.org> # v6.6+
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_blit.c | 35 +++++++++++++++--
> drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 7 ++--
> drivers/gpu/drm/vmwgfx/vmwgfx_bo.h | 2 +
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 1 +
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 3 ++
> drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 32 ++++++++++++++++
> drivers/gpu/drm/vmwgfx/vmwgfx_prime.c | 15 +++++++-
> drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 44 +++++++++++++++-------
> 8 files changed, 117 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_blit.c b/drivers/gpu/drm/vmwgfx/vmwgfx_blit.c
> index c52c7bf1485b..717d624e9a05 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_blit.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_blit.c
> @@ -456,8 +456,10 @@ int vmw_bo_cpu_blit(struct ttm_buffer_object *dst,
> .no_wait_gpu = false
> };
> u32 j, initial_line = dst_offset / dst_stride;
> - struct vmw_bo_blit_line_data d;
> + struct vmw_bo_blit_line_data d = {0};
> int ret = 0;
> + struct page **dst_pages = NULL;
> + struct page **src_pages = NULL;
>
> /* Buffer objects need to be either pinned or reserved: */
> if (!(dst->pin_count))
> @@ -477,12 +479,35 @@ int vmw_bo_cpu_blit(struct ttm_buffer_object *dst,
> return ret;
> }
>
> + if (!src->ttm->pages && src->ttm->sg) {
> + src_pages = kvmalloc_array(src->ttm->num_pages,
> + sizeof(struct page *), GFP_KERNEL);
> + if (!src_pages)
> + return -ENOMEM;
> + ret = drm_prime_sg_to_page_array(src->ttm->sg, src_pages,
> + src->ttm->num_pages);
> + if (ret)
> + goto out;
> + }
> + if (!dst->ttm->pages && dst->ttm->sg) {
> + dst_pages = kvmalloc_array(dst->ttm->num_pages,
> + sizeof(struct page *), GFP_KERNEL);
> + if (!dst_pages) {
> + ret = -ENOMEM;
> + goto out;
> + }
> + ret = drm_prime_sg_to_page_array(dst->ttm->sg, dst_pages,
> + dst->ttm->num_pages);
> + if (ret)
> + goto out;
> + }
> +
> d.mapped_dst = 0;
> d.mapped_src = 0;
> d.dst_addr = NULL;
> d.src_addr = NULL;
> - d.dst_pages = dst->ttm->pages;
> - d.src_pages = src->ttm->pages;
> + d.dst_pages = dst->ttm->pages ? dst->ttm->pages : dst_pages;
> + d.src_pages = src->ttm->pages ? src->ttm->pages : src_pages;
> d.dst_num_pages = PFN_UP(dst->resource->size);
> d.src_num_pages = PFN_UP(src->resource->size);
> d.dst_prot = ttm_io_prot(dst, dst->resource, PAGE_KERNEL);
> @@ -504,6 +529,10 @@ int vmw_bo_cpu_blit(struct ttm_buffer_object *dst,
> kunmap_atomic(d.src_addr);
> if (d.dst_addr)
> kunmap_atomic(d.dst_addr);
> + if (src_pages)
> + kvfree(src_pages);
> + if (dst_pages)
> + kvfree(dst_pages);
>
> return ret;
> }
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> index bfd41ce3c8f4..e5eb21a471a6 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> @@ -377,7 +377,8 @@ static int vmw_bo_init(struct vmw_private *dev_priv,
> {
> struct ttm_operation_ctx ctx = {
> .interruptible = params->bo_type != ttm_bo_type_kernel,
> - .no_wait_gpu = false
> + .no_wait_gpu = false,
> + .resv = params->resv,
> };
> struct ttm_device *bdev = &dev_priv->bdev;
> struct drm_device *vdev = &dev_priv->drm;
> @@ -394,8 +395,8 @@ static int vmw_bo_init(struct vmw_private *dev_priv,
>
> vmw_bo_placement_set(vmw_bo, params->domain, params->busy_domain);
> ret = ttm_bo_init_reserved(bdev, &vmw_bo->tbo, params->bo_type,
> - &vmw_bo->placement, 0, &ctx, NULL,
> - NULL, destroy);
> + &vmw_bo->placement, 0, &ctx,
> + params->sg, params->resv, destroy);
> if (unlikely(ret))
> return ret;
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.h b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.h
> index 0d496dc9c6af..f349642e6190 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.h
> @@ -55,6 +55,8 @@ struct vmw_bo_params {
> enum ttm_bo_type bo_type;
> size_t size;
> bool pin;
> + struct dma_resv *resv;
> + struct sg_table *sg;
> };
>
> /**
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index 89d3679d2608..41ad13e45554 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -1631,6 +1631,7 @@ static const struct drm_driver driver = {
>
> .prime_fd_to_handle = vmw_prime_fd_to_handle,
> .prime_handle_to_fd = vmw_prime_handle_to_fd,
> + .gem_prime_import_sg_table = vmw_prime_import_sg_table,
>
> .fops = &vmwgfx_driver_fops,
> .name = VMWGFX_DRIVER_NAME,
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> index ddbceaa31b59..4ecaea0026fc 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> @@ -1107,6 +1107,9 @@ extern int vmw_prime_handle_to_fd(struct drm_device *dev,
> struct drm_file *file_priv,
> uint32_t handle, uint32_t flags,
> int *prime_fd);
> +struct drm_gem_object *vmw_prime_import_sg_table(struct drm_device *dev,
> + struct dma_buf_attachment *attach,
> + struct sg_table *table);
>
> /*
> * MemoryOBject management - vmwgfx_mob.c
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
> index 186150f41fbc..2132a8ad8c0c 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
> @@ -136,6 +136,38 @@ int vmw_gem_object_create_with_handle(struct vmw_private *dev_priv,
> return ret;
> }
>
> +struct drm_gem_object *vmw_prime_import_sg_table(struct drm_device *dev,
> + struct dma_buf_attachment *attach,
> + struct sg_table *table)
> +{
> + int ret;
> + struct vmw_private *dev_priv = vmw_priv(dev);
> + struct drm_gem_object *gem = NULL;
> + struct vmw_bo *vbo;
> + struct vmw_bo_params params = {
> + .domain = (dev_priv->has_mob) ? VMW_BO_DOMAIN_SYS : VMW_BO_DOMAIN_VRAM,
> + .busy_domain = VMW_BO_DOMAIN_SYS,
> + .bo_type = ttm_bo_type_sg,
> + .size = attach->dmabuf->size,
> + .pin = false,
> + .resv = attach->dmabuf->resv,
> + .sg = table,
> +
> + };
> +
> + dma_resv_lock(params.resv, NULL);
> +
> + ret = vmw_bo_create(dev_priv, ¶ms, &vbo);
> + if (ret != 0)
> + goto out_no_bo;
> +
> + vbo->tbo.base.funcs = &vmw_gem_object_funcs;
> +
> + gem = &vbo->tbo.base;
> +out_no_bo:
> + dma_resv_unlock(params.resv);
> + return gem;
> +}
>
> int vmw_gem_object_create_ioctl(struct drm_device *dev, void *data,
> struct drm_file *filp)
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c b/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c
> index 2d72a5ee7c0c..c99cad444991 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c
> @@ -75,8 +75,12 @@ int vmw_prime_fd_to_handle(struct drm_device *dev,
> int fd, u32 *handle)
> {
> struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile;
> + int ret = ttm_prime_fd_to_handle(tfile, fd, handle);
>
> - return ttm_prime_fd_to_handle(tfile, fd, handle);
> + if (ret)
> + ret = drm_gem_prime_fd_to_handle(dev, file_priv, fd, handle);
> +
> + return ret;
> }
>
> int vmw_prime_handle_to_fd(struct drm_device *dev,
> @@ -85,5 +89,12 @@ int vmw_prime_handle_to_fd(struct drm_device *dev,
> int *prime_fd)
> {
> struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile;
> - return ttm_prime_handle_to_fd(tfile, handle, flags, prime_fd);
> + int ret;
> +
> + if (handle > VMWGFX_NUM_MOB)
> + ret = ttm_prime_handle_to_fd(tfile, handle, flags, prime_fd);
> + else
> + ret = drm_gem_prime_handle_to_fd(dev, file_priv, handle, flags, prime_fd);
> +
> + return ret;
> }
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
> index 4d23d0a70bcb..621d98b376bb 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
> @@ -188,13 +188,18 @@ static int vmw_ttm_map_dma(struct vmw_ttm_tt *vmw_tt)
> switch (dev_priv->map_mode) {
> case vmw_dma_map_bind:
> case vmw_dma_map_populate:
> - vsgt->sgt = &vmw_tt->sgt;
> - ret = sg_alloc_table_from_pages_segment(
> - &vmw_tt->sgt, vsgt->pages, vsgt->num_pages, 0,
> - (unsigned long)vsgt->num_pages << PAGE_SHIFT,
> - dma_get_max_seg_size(dev_priv->drm.dev), GFP_KERNEL);
> - if (ret)
> - goto out_sg_alloc_fail;
> + if (vmw_tt->dma_ttm.page_flags & TTM_TT_FLAG_EXTERNAL) {
> + vsgt->sgt = vmw_tt->dma_ttm.sg;
> + } else {
> + vsgt->sgt = &vmw_tt->sgt;
> + ret = sg_alloc_table_from_pages_segment(&vmw_tt->sgt,
> + vsgt->pages, vsgt->num_pages, 0,
> + (unsigned long)vsgt->num_pages << PAGE_SHIFT,
> + dma_get_max_seg_size(dev_priv->drm.dev),
> + GFP_KERNEL);
> + if (ret)
> + goto out_sg_alloc_fail;
> + }
>
> ret = vmw_ttm_map_for_dma(vmw_tt);
> if (unlikely(ret != 0))
> @@ -209,8 +214,9 @@ static int vmw_ttm_map_dma(struct vmw_ttm_tt *vmw_tt)
> return 0;
>
> out_map_fail:
> - sg_free_table(vmw_tt->vsgt.sgt);
> - vmw_tt->vsgt.sgt = NULL;
> + drm_warn(&dev_priv->drm, "VSG table map failed!");
> + sg_free_table(vsgt->sgt);
> + vsgt->sgt = NULL;
> out_sg_alloc_fail:
> return ret;
> }
> @@ -356,15 +362,17 @@ static void vmw_ttm_destroy(struct ttm_device *bdev, struct ttm_tt *ttm)
> static int vmw_ttm_populate(struct ttm_device *bdev,
> struct ttm_tt *ttm, struct ttm_operation_ctx *ctx)
> {
> - int ret;
> + bool external = (ttm->page_flags & TTM_TT_FLAG_EXTERNAL) != 0;
>
> - /* TODO: maybe completely drop this ? */
> if (ttm_tt_is_populated(ttm))
> return 0;
>
> - ret = ttm_pool_alloc(&bdev->pool, ttm, ctx);
> + if (external && ttm->sg)
> + return drm_prime_sg_to_dma_addr_array(ttm->sg,
> + ttm->dma_address,
> + ttm->num_pages);
>
> - return ret;
> + return ttm_pool_alloc(&bdev->pool, ttm, ctx);
> }
>
> static void vmw_ttm_unpopulate(struct ttm_device *bdev,
> @@ -372,6 +380,10 @@ static void vmw_ttm_unpopulate(struct ttm_device *bdev,
> {
> struct vmw_ttm_tt *vmw_tt = container_of(ttm, struct vmw_ttm_tt,
> dma_ttm);
> + bool external = (ttm->page_flags & TTM_TT_FLAG_EXTERNAL) != 0;
> +
> + if (external)
> + return;
>
> vmw_ttm_unbind(bdev, ttm);
>
> @@ -390,6 +402,7 @@ static struct ttm_tt *vmw_ttm_tt_create(struct ttm_buffer_object *bo,
> {
> struct vmw_ttm_tt *vmw_be;
> int ret;
> + bool external = bo->type == ttm_bo_type_sg;
>
> vmw_be = kzalloc(sizeof(*vmw_be), GFP_KERNEL);
> if (!vmw_be)
> @@ -398,7 +411,10 @@ static struct ttm_tt *vmw_ttm_tt_create(struct ttm_buffer_object *bo,
> vmw_be->dev_priv = vmw_priv_from_ttm(bo->bdev);
> vmw_be->mob = NULL;
>
> - if (vmw_be->dev_priv->map_mode == vmw_dma_alloc_coherent)
> + if (external)
> + page_flags |= TTM_TT_FLAG_EXTERNAL | TTM_TT_FLAG_EXTERNAL_MAPPABLE;
> +
> + if (vmw_be->dev_priv->map_mode == vmw_dma_alloc_coherent || external)
> ret = ttm_sg_tt_init(&vmw_be->dma_ttm, bo, page_flags,
> ttm_cached);
> else
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2024-04-09 15:30 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-02 23:28 [PATCH 0/5] drm/vmwgfx: vblank and crc generation support Zack Rusin
2024-04-02 23:28 ` [PATCH 1/5] drm/vmwgfx: Implement virtual kms Zack Rusin
2024-04-05 21:53 ` Maaz Mombasawala
2024-04-05 22:26 ` Zack Rusin
2024-04-02 23:28 ` [PATCH 2/5] drm/vmwgfx: Implement virtual crc generation Zack Rusin
2024-04-03 11:31 ` kernel test robot
2024-04-09 15:08 ` Martin Krastev
2024-04-02 23:28 ` [PATCH 3/5] drm/vmwgfx: Fix prime import/export Zack Rusin
2024-04-09 15:30 ` Martin Krastev
2024-04-02 23:28 ` [PATCH 4/5] drm/vmwgfx: Fix crtc's atomic check conditional Zack Rusin
2024-04-05 19:02 ` Ian Forbes
2024-04-09 15:12 ` Martin Krastev
2024-04-02 23:28 ` [PATCH 5/5] drm/vmwgfx: Sort primary plane formats by order of preference Zack Rusin
2024-04-03 7:42 ` Pekka Paalanen
2024-04-03 11:44 ` Zack Rusin
2024-04-04 8:18 ` Pekka Paalanen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.