Re: Assistance with a problem related to GEM and Atomic Commit inside vkms

On Wed, Apr 03, 2019 at 04:35:11PM -0300, Rodrigo Siqueira wrote:

Hi,

Hi Rodrigo,

I’m working to add writeback support to vkms; you can see my current patch at the end of this email (some prints highlight the issue that I’m asking for help here). I’m using the Liviu Dudau and Brian Starkey IGT patchset, which can be seen here: https://patchwork.freedesktop.org/series/39229/.

I’m stuck with a problem that happens in my vkms_wb_atomic_commit() function, which is a helper function at drm_connector_helper_funcs. You can see the full implementation of this function at the end of this email, but here is the important part:

struct drm_framebuffer *fb = conn_state->writeback_job->fb; struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0); struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(gem_obj);

A few lines after this variable initialization, I try to make a memory copy like this:

memcpy(vkms_conn->writeback_vaddr, vkms_obj->vaddr, vkms_obj->gem.size);

If I test my implementation with the subtest writeback-fb-id, everything work as expected; for extra details, this is the dmesg output after this test and with a debug print:

=> writeback-fb-id

[Apr 3 11:30] CIFS: Attempting to mount //10.0.2.4/qemu [ +1.601900] Console: switching to colour dummy device 80x25 [ +0.000017] [IGT] kms_writeback: executing [ +0.032532] [IGT] kms_writeback: starting subtest writeback-fb-id [ +0.001932] VKMS_OBJ (1): Buff (1) - Vaddr (1) - Size = 1228800 - count = 1 [ +0.050524] [IGT] kms_writeback: exiting, ret=0 [ +0.005202] Console: switching to colour frame buffer device 100x37

In the above log, you can see the print info; notice that Vaddr is the output of vkms_obj->vaddr (Vaddr -> vkms_obj->vaddr != NULL). On the other hand, if I try the subtest writeback-check-output, something weird happens in this function. Here is the dmesg output for this test:

=> writeback-check-output

[ +29.911185] Console: switching to colour dummy device 80x25 [ +0.000017] [IGT] kms_writeback: executing [ +0.021289] [IGT] kms_writeback: starting subtest writeback-check-output [ +0.016270] VKMS_OBJ (1): Buff (1) - Vaddr (0) - Size = 1228800 - count = 0 [ +0.000005] BUG: unable to handle kernel NULL pointer dereference at 0000000000000000 [ +0.000004] #PF error: [normal kernel read fault] [ +0.000001] PGD 0 P4D 0 [ +0.000003] Oops: 0000 [#1] PREEMPT SMP PTI [ +0.000002] CPU: 3 PID: 426 Comm: kms_writeback Not tainted 5.0.0-VKMS-RULES+ #102 [ +0.000002] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-20181126_142135-anatol 04/01/2014 [ +0.000021] RIP: 0010:__memcpy+0x12/0x20 [ +0.000002] Code: ff 0f 31 48 c1 e2 20 48 09 c2 48 31 d3 e9 79 ff ff ff 90 90 90 90 90 90 0f 1f 44 00 00 48 89 f8 48 89 d1 48 c1 e9 03 83 e2 07 <f3> 48 a5 89 d1 f3 a4 c3 66 0f 1f 44 00 00 48 89 f8 48 89 d1 f3 a4 [ +0.000001] RSP: 0018:ffffadaf0098fbd8 EFLAGS: 00010246 [ +0.000002] RAX: ffff9f1bf5400000 RBX: ffff9f1bfa28df00 RCX: 0000000000025800 [ +0.000001] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff9f1bf5400000 [ +0.000001] RBP: 0000000000000001 R08: 00000017818788be R09: ffffadaf0098fb60 [ +0.000001] R10: ffff9f1bfffdc998 R11: ffffffffa25ec401 R12: ffff9f1b777ff430 [ +0.000001] R13: ffffffffc06558b8 R14: ffffffffc06853c0 R15: ffffffffc0685520 [ +0.000002] FS: 00007f2862deae80(0000) GS:ffff9f1bfc980000(0000) knlGS:0000000000000000 [ +0.000001] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ +0.000001] CR2: 0000000000000000 CR3: 00000000baf6c000 CR4: 00000000000006e0 [ +0.000004] Call Trace: [ +0.000023] drm_atomic_helper_commit_modeset_enables+0x15d/0x1e0 [drm_kms_helper] [ +0.000028] drm_atomic_helper_commit_tail+0x31/0x60 [drm_kms_helper] [ +0.000008] commit_tail+0x58/0x70 [drm_kms_helper] [ +0.000008] drm_atomic_helper_commit+0x103/0x110 [drm_kms_helper] [ +0.000040] drm_mode_atomic_ioctl+0x829/0x950 [drm] [ +0.000012] ? drm_atomic_set_property+0x960/0x960 [drm] [ +0.000029] drm_ioctl_kernel+0xb2/0xf0 [drm] [ +0.000011] drm_ioctl+0x25f/0x3f0 [drm] [ +0.000009] ? drm_atomic_set_property+0x960/0x960 [drm] [ +0.000004] ? n_tty_open+0xa0/0xa0 [ +0.000004] do_vfs_ioctl+0xa4/0x630 [ +0.000003] ksys_ioctl+0x60/0x90 [ +0.000003] ? ksys_write+0x4f/0xb0 [ +0.000002] __x64_sys_ioctl+0x16/0x20 [ +0.000003] do_syscall_64+0x5b/0x170 [ +0.000003] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ +0.000002] RIP: 0033:0x7f286677580b [ +0.000001] Code: 0f 1e fa 48 8b 05 55 b6 0c 00 64 c7 00 26 00 00 00 48 c7 c0 ff

This call stack looks strange! drm_atomic_set_property() (if that is right function being called) should not end up calling drm_atomic_helper_commit(). I would start by trying to get the actual call function trace to see what is going on.

Best regards, Liviu

Notice that Vaddr became NULL for some reason that I cannot understand. From the userspace perspective it looks like that drmModeAtomicCommit() is the entry point for this problem. I tried to debug many things related to GEM and atomic commit, and the only thing that is worth to highlight here is the fact that vkms_gem_fault() function is invoked 1500 times when the test writeback-check-output is executed.

After many attempts to try to find out the root of the problem, I’m out of ideas. In this sense, I would like to know if anyone can help by shedding some light on any issues I'm missing or by pointing at any other direction.

Best Regards Rodrigo Siqueira

---

drivers/gpu/drm/vkms/vkms_crtc.c | 5 + drivers/gpu/drm/vkms/vkms_drv.c | 8 ++ drivers/gpu/drm/vkms/vkms_drv.h | 27 ++++++ drivers/gpu/drm/vkms/vkms_gem.c | 2 +- drivers/gpu/drm/vkms/vkms_output.c | 145 ++++++++++++++++++++++++++++- drivers/gpu/drm/vkms/vkms_plane.c | 2 +- 6 files changed, 184 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c index 8a9aeb0a9ea8..e54265a1c67d 100644 --- a/drivers/gpu/drm/vkms/vkms_crtc.c +++ b/drivers/gpu/drm/vkms/vkms_crtc.c @@ -19,6 +19,11 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) if (!ret) DRM_ERROR("vkms failure on handling vblank");

• if (output->wb_state == MW_ONESHOT) {

• drm_writeback_signal_completion(&output->wb_connector, 0);
  

• output->wb_state = MW_STOP;
  

• }
• if (state && output->crc_enabled) { u64 frame = drm_crtc_accurate_vblank_count(crtc);

diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c index 738dd6206d85..e0d7f94e3972 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.c +++ b/drivers/gpu/drm/vkms/vkms_drv.c @@ -29,6 +29,10 @@ bool enable_cursor; module_param_named(enable_cursor, enable_cursor, bool, 0444); MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");

+int enable_writeback; +module_param_named(enable_writeback, enable_writeback, int, 0444); +MODULE_PARM_DESC(enable_writeback, "Enable/Disable writeback connector");

static const struct file_operations vkms_driver_fops = { .owner = THIS_MODULE, .open = drm_open, @@ -123,6 +127,10 @@ static int __init vkms_init(void) goto out_fini; }

• vkms_device->output.writeback_enabled = enable_writeback;
• if (enable_writeback)

• DRM_INFO("Writeback connector enabled");
  

• ret = vkms_modeset_init(vkms_device); if (ret) goto out_fini;

diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index 81f1cfbeb936..d11dcf6b5d55 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.h +++ b/drivers/gpu/drm/vkms/vkms_drv.h @@ -8,6 +8,7 @@ #include <drm/drm_gem.h> #include <drm/drm_encoder.h> #include <linux/hrtimer.h> +#include <drm/drm_writeback.h>

#define XRES_MIN 20 #define YRES_MIN 20 @@ -20,6 +21,11 @@

extern bool enable_cursor;

+enum {

• MW_STOP = 0,
• MW_ONESHOT,

+};

static const u32 vkms_formats[] = { DRM_FORMAT_XRGB8888, }; @@ -46,6 +52,16 @@ struct vkms_plane_state { struct vkms_crc_data *crc_data; };

+/**

• • vkms_connector_state - Driver connector specific state
• • @base: base connector state
• • @writeback_vaddr: keep track for writeback framebuffer destination
• */

+struct vkms_connector_state {

• struct drm_connector_state base;
• void *writeback_vaddr;

+};

/**

• vkms_crtc_state - Driver specific CRTC state
• @base: base CRTC state

@@ -64,10 +80,13 @@ struct vkms_output { struct drm_crtc crtc; struct drm_encoder encoder; struct drm_connector connector;

• struct drm_writeback_connector wb_connector; struct hrtimer vblank_hrtimer; ktime_t period_ns; struct drm_pending_vblank_event *event; bool crc_enabled;
• int writeback_enabled;
• u8 wb_state; /* ordered wq for crc_work */ struct workqueue_struct *crc_workq; /* protects concurrent access to crc_data */

@@ -93,6 +112,12 @@ struct vkms_gem_object { #define drm_crtc_to_vkms_output(target) \ container_of(target, struct vkms_output, crtc)

+#define drm_connector_to_wb_connector(target) \

• container_of(target, struct drm_writeback_connector, base)

+#define drm_wb_to_vkms_output(target) \

• container_of(target, struct vkms_output, wb_connector)

#define drm_device_to_vkms_device(target) \ container_of(target, struct vkms_device, drm)

@@ -105,6 +130,8 @@ struct vkms_gem_object { #define to_vkms_plane_state(target)\ container_of(target, struct vkms_plane_state, base)

+#define to_wb_state(_state) (struct vkms_connector_state *)(_state)

/* CRTC */ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, struct drm_plane *primary, struct drm_plane *cursor); diff --git a/drivers/gpu/drm/vkms/vkms_gem.c b/drivers/gpu/drm/vkms/vkms_gem.c index 138b0bb325cf..9e3d25de6aba 100644 --- a/drivers/gpu/drm/vkms/vkms_gem.c +++ b/drivers/gpu/drm/vkms/vkms_gem.c @@ -51,7 +51,7 @@ vm_fault_t vkms_gem_fault(struct vm_fault *vmf) page_offset = (vaddr - vma->vm_start) >> PAGE_SHIFT; num_pages = DIV_ROUND_UP(obj->gem.size, PAGE_SIZE);

• if (page_offset > num_pages)

• if (page_offset >= num_pages) return VM_FAULT_SIGBUS;

  mutex_lock(&obj->pages_lock);

diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c index 3b162b25312e..66b25c380f21 100644 --- a/drivers/gpu/drm/vkms/vkms_output.c +++ b/drivers/gpu/drm/vkms/vkms_output.c @@ -3,6 +3,8 @@ #include "vkms_drv.h" #include <drm/drm_atomic_helper.h> #include <drm/drm_probe_helper.h> +#include <drm/drm_writeback.h> +#include <drm/drm_gem_framebuffer_helper.h>

static void vkms_connector_destroy(struct drm_connector *connector) { @@ -10,6 +12,51 @@ static void vkms_connector_destroy(struct drm_connector *connector) drm_connector_cleanup(connector); }

+static void vkms_wb_connector_reset(struct drm_connector *connector) +{

• struct vkms_connector_state *wb_state;
• wb_state = kzalloc(sizeof(*wb_state), GFP_KERNEL);
• if (!wb_state) {

• DRM_ERROR("Failed to allocate memory for connector state");
  

• return;
  

• }
• if (connector->state)

• __drm_atomic_helper_connector_destroy_state(connector->state);
  

• kfree(connector->state);
• __drm_atomic_helper_connector_reset(connector, &wb_state->base);

+}

+static struct drm_connector_state * +vkms_wb_connector_duplicate_state(struct drm_connector *connector) +{

• struct vkms_connector_state *wb_state, *wb_current_state;
• if (WARN_ON(!connector->state))

• return NULL;
  

• wb_state = kzalloc(sizeof(*wb_state), GFP_KERNEL);
• if (!wb_state)

• return NULL;
  

• wb_current_state = to_wb_state(connector->state);
• wb_state->writeback_vaddr = wb_current_state->writeback_vaddr;
• __drm_atomic_helper_connector_duplicate_state(connector, &wb_state->base);
• return &wb_state->base;

+}

+static const struct drm_connector_funcs vkms_wb_connector_funcs = {

• .fill_modes = drm_helper_probe_single_connector_modes,
• .destroy = vkms_connector_destroy,
• .reset = vkms_wb_connector_reset,
• .atomic_duplicate_state = vkms_wb_connector_duplicate_state,
• .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,

+};

static const struct drm_connector_funcs vkms_connector_funcs = { .fill_modes = drm_helper_probe_single_connector_modes, .destroy = vkms_connector_destroy, @@ -18,8 +65,37 @@ static const struct drm_connector_funcs vkms_connector_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, };

-static const struct drm_encoder_funcs vkms_encoder_funcs = {

• .destroy = drm_encoder_cleanup,

+static int vkms_wb_encoder_atomic_check(struct drm_encoder *encoder,

• 			struct drm_crtc_state *crtc_state,
  

• 			struct drm_connector_state *conn_state)
  

+{

• struct drm_framebuffer *fb;
• if (!conn_state->writeback_job || !conn_state->writeback_job->fb)

• return 0;
  

• fb = conn_state->writeback_job->fb;
• if (fb->width != crtc_state->mode.hdisplay ||

•    fb->height != crtc_state->mode.vdisplay) {
  

• DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
  

• 	      fb->width, fb->height);
  

• return -EINVAL;
  

• }
• if (fb->format->format != DRM_FORMAT_XRGB8888) {

• struct drm_format_name_buf format_name;
  

• DRM_DEBUG_KMS("Invalid pixel format %s\n",
  

• 	      drm_get_format_name(fb->format->format,
  

• 				  &format_name));
  

• return -EINVAL;
  

• }
• return 0;

+}

+static const struct drm_encoder_helper_funcs vkms_wb_encoder_helper_funcs = {

• .atomic_check = vkms_wb_encoder_atomic_check,

};

static int vkms_conn_get_modes(struct drm_connector *connector) @@ -32,10 +108,66 @@ static int vkms_conn_get_modes(struct drm_connector *connector) return count; }

+static void vkms_wb_atomic_commit(struct drm_connector *conn,

• 		  struct drm_connector_state *state)
  

+{

• struct vkms_device *vkmsdev = drm_device_to_vkms_device(conn->dev);
• struct vkms_output *output = &vkmsdev->output;
• struct drm_writeback_connector *wb_conn = &output->wb_connector;
• struct drm_connector_state *conn_state = wb_conn->base.state;
• struct vkms_connector_state *vkms_conn = to_wb_state(conn_state);
• if (!conn_state)

• return;
  

• if (conn_state->writeback_job && conn_state->writeback_job->fb) {

• struct drm_framebuffer *fb = conn_state->writeback_job->fb;
  

• struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
  

• struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(gem_obj);
  

• vkms_conn->writeback_vaddr = kzalloc(vkms_obj->gem.size, GFP_KERNEL);
  

• drm_writeback_queue_job(wb_conn, conn_state->writeback_job);
  

• conn_state->writeback_job = NULL;
  

• output->wb_state = MW_ONESHOT;
  

+pr_info("VKMS_OBJ (%d): Buff (%d) - Vaddr (%d) - Size = %ld - count = %d", vkms_obj != NULL,

• vkms_conn->writeback_vaddr != NULL, vkms_obj->vaddr != NULL, vkms_obj->gem.size,
• vkms_obj->vmap_count);

• memcpy(vkms_conn->writeback_vaddr, vkms_obj->vaddr, vkms_obj->gem.size);
  

• } else {

• output->wb_state = MW_STOP;
  

• }

+}

static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = { .get_modes = vkms_conn_get_modes, };

+static const struct drm_encoder_funcs vkms_encoder_funcs = {

• .destroy = drm_encoder_cleanup,

+};

+static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {

• .get_modes = vkms_conn_get_modes,
• .atomic_commit = vkms_wb_atomic_commit,

+};

+static int enable_writeback_connector(struct vkms_device *vkmsdev) +{

• struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
• vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
• drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
• return drm_writeback_connector_init(&vkmsdev->drm, wb,

• 			    &vkms_wb_connector_funcs,
  

• 			    &vkms_wb_encoder_helper_funcs,
  

• 			    vkms_formats,
  

• 			    ARRAY_SIZE(vkms_formats));
  

+}

int vkms_output_init(struct vkms_device *vkmsdev) { struct vkms_output *output = &vkmsdev->output; @@ -77,13 +209,14 @@ int vkms_output_init(struct vkms_device *vkmsdev) goto err_connector_register; }

• encoder->possible_crtcs = 1;
• ret = drm_encoder_init(dev, encoder, &vkms_encoder_funcs, DRM_MODE_ENCODER_VIRTUAL, NULL); if (ret) { DRM_ERROR("Failed to init encoder\n"); goto err_encoder; }

• encoder->possible_crtcs = 1;

  ret = drm_connector_attach_encoder(connector, encoder); if (ret) {

@@ -91,6 +224,12 @@ int vkms_output_init(struct vkms_device *vkmsdev) goto err_attach; }

• if (vkmsdev->output.writeback_enabled) {

• ret = enable_writeback_connector(vkmsdev);
  

• if (ret)
  

• 	DRM_ERROR("Failed to init writeback connector\n");
  

• }

• drm_mode_config_reset(dev);

  return 0;

diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c index 0e67d2d42f0c..14259ffe47d8 100644 --- a/drivers/gpu/drm/vkms/vkms_plane.c +++ b/drivers/gpu/drm/vkms/vkms_plane.c @@ -190,7 +190,7 @@ struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev, funcs = &vkms_primary_helper_funcs; }

• ret = drm_universal_plane_init(dev, plane, 0,

• ret = drm_universal_plane_init(dev, plane, 1, &vkms_plane_funcs, formats, nformats, NULL, type, NULL);

-- 2.21.0

-- Rodrigo Siqueira https://siqueira.tech