Assistance with a problem related to GEM and Atomic Commit inside vkms
Liviu Dudau
Liviu.Dudau at arm.com
Fri Apr 5 15:20:18 UTC 2019
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
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
More information about the dri-devel
mailing list