From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rodrigo Siqueira Subject: Assistance with a problem related to GEM and Atomic Commit inside vkms Date: Wed, 3 Apr 2019 16:35:11 -0300 Message-ID: <20190403193511.4waspuspncu7xrn7@smtp.gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2027743196==" Return-path: Received: from mail-qt1-x842.google.com (mail-qt1-x842.google.com [IPv6:2607:f8b0:4864:20::842]) by gabe.freedesktop.org (Postfix) with ESMTPS id 360906E41D for ; Wed, 3 Apr 2019 19:35:16 +0000 (UTC) Received: by mail-qt1-x842.google.com with SMTP id w30so393441qta.8 for ; Wed, 03 Apr 2019 12:35:16 -0700 (PDT) List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Brian Starkey , Liviu Dudau , Daniel Vetter Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============2027743196== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="ddamgngzb27wv7ig" Content-Disposition: inline --ddamgngzb27wv7ig Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, I=E2=80=99m 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=E2= =80=99m asking for help here). I=E2=80=99m using the Liviu Dudau and Brian Starkey = IGT patchset, which can be seen here: https://patchwork.freedesktop.org/series/39229/.=20 I=E2=80=99m 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 =3D conn_state->writeback_job->fb; struct drm_gem_object *gem_obj =3D drm_gem_fb_get_obj(fb, 0); struct vkms_gem_object *vkms_obj =3D 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: =3D> 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 =3D 1228800 - count= =3D 1 [ +0.050524] [IGT] kms_writeback: exiting, ret=3D0 [ +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 !=3D 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: =3D> 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 =3D 1228800 - count= =3D 0 [ +0.000005] BUG: unable to handle kernel NULL pointer dereference at 0000= 000000000000 [ +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-RU= LES+ #102 [ +0.000002] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1= =2E12.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 9= 0 90 90 90 90 90 0f 1f 44 00 00 48 89 f8 48 89 d1 48 c1 e9 03 83 e2 07 = 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: 000000000002= 5800 [ +0.000001] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff9f1bf540= 0000 [ +0.000001] RBP: 0000000000000001 R08: 00000017818788be R09: ffffadaf0098= fb60 [ +0.000001] R10: ffff9f1bfffdc998 R11: ffffffffa25ec401 R12: ffff9f1b777f= f430 [ +0.000001] R13: ffffffffc06558b8 R14: ffffffffc06853c0 R15: ffffffffc068= 5520 [ +0.000002] FS: 00007f2862deae80(0000) GS:ffff9f1bfc980000(0000) knlGS:0= 000000000000000 [ +0.000001] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ +0.000001] CR2: 0000000000000000 CR3: 00000000baf6c000 CR4: 000000000000= 06e0 [ +0.000004] Call Trace: [ +0.000023] drm_atomic_helper_commit_modeset_enables+0x15d/0x1e0 [drm_km= s_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 c= 7 c0 ff=20 Notice that Vaddr became NULL for some reason that I cannot understand. =46rom 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=E2=80=99m= 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_c= rtc.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"); =20 + if (output->wb_state =3D=3D MW_ONESHOT) { + drm_writeback_signal_completion(&output->wb_connector, 0); + output->wb_state =3D MW_STOP; + } + if (state && output->crc_enabled) { u64 frame =3D drm_crtc_accurate_vblank_count(crtc); =20 diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_dr= v.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"); =20 +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 =3D { .owner =3D THIS_MODULE, .open =3D drm_open, @@ -123,6 +127,10 @@ static int __init vkms_init(void) goto out_fini; } =20 + vkms_device->output.writeback_enabled =3D enable_writeback; + if (enable_writeback) + DRM_INFO("Writeback connector enabled"); + ret =3D 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_dr= v.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 #include #include +#include =20 #define XRES_MIN 20 #define YRES_MIN 20 @@ -20,6 +21,11 @@ =20 extern bool enable_cursor; =20 +enum { + MW_STOP =3D 0, + MW_ONESHOT, +}; + static const u32 vkms_formats[] =3D { DRM_FORMAT_XRGB8888, }; @@ -46,6 +52,16 @@ struct vkms_plane_state { struct vkms_crc_data *crc_data; }; =20 +/** + * 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) =20 +#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) =20 @@ -105,6 +130,8 @@ struct vkms_gem_object { #define to_vkms_plane_state(target)\ container_of(target, struct vkms_plane_state, base) =20 +#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_ge= m.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 =3D (vaddr - vma->vm_start) >> PAGE_SHIFT; num_pages =3D DIV_ROUND_UP(obj->gem.size, PAGE_SIZE); =20 - if (page_offset > num_pages) + if (page_offset >=3D num_pages) return VM_FAULT_SIGBUS; =20 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 #include +#include +#include =20 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); } =20 +static void vkms_wb_connector_reset(struct drm_connector *connector) +{ + struct vkms_connector_state *wb_state; + + wb_state =3D 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 =3D kzalloc(sizeof(*wb_state), GFP_KERNEL); + if (!wb_state) + return NULL; + + wb_current_state =3D to_wb_state(connector->state); + wb_state->writeback_vaddr =3D 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 =3D { + .fill_modes =3D drm_helper_probe_single_connector_modes, + .destroy =3D vkms_connector_destroy, + .reset =3D vkms_wb_connector_reset, + .atomic_duplicate_state =3D vkms_wb_connector_duplicate_state, + .atomic_destroy_state =3D drm_atomic_helper_connector_destroy_state, +}; + static const struct drm_connector_funcs vkms_connector_funcs =3D { .fill_modes =3D drm_helper_probe_single_connector_modes, .destroy =3D vkms_connector_destroy, @@ -18,8 +65,37 @@ static const struct drm_connector_funcs vkms_connector_f= uncs =3D { .atomic_destroy_state =3D drm_atomic_helper_connector_destroy_state, }; =20 -static const struct drm_encoder_funcs vkms_encoder_funcs =3D { - .destroy =3D 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 =3D conn_state->writeback_job->fb; + if (fb->width !=3D crtc_state->mode.hdisplay || + fb->height !=3D crtc_state->mode.vdisplay) { + DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n", + fb->width, fb->height); + return -EINVAL; + } + + if (fb->format->format !=3D 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 = =3D { + .atomic_check =3D vkms_wb_encoder_atomic_check, }; =20 static int vkms_conn_get_modes(struct drm_connector *connector) @@ -32,10 +108,66 @@ static int vkms_conn_get_modes(struct drm_connector *c= onnector) return count; } =20 +static void vkms_wb_atomic_commit(struct drm_connector *conn, + struct drm_connector_state *state) +{ + struct vkms_device *vkmsdev =3D drm_device_to_vkms_device(conn->dev); + struct vkms_output *output =3D &vkmsdev->output; + struct drm_writeback_connector *wb_conn =3D &output->wb_connector; + struct drm_connector_state *conn_state =3D wb_conn->base.state; + struct vkms_connector_state *vkms_conn =3D to_wb_state(conn_state); + + if (!conn_state) + return; + + if (conn_state->writeback_job && conn_state->writeback_job->fb) { + struct drm_framebuffer *fb =3D conn_state->writeback_job->fb; + struct drm_gem_object *gem_obj =3D drm_gem_fb_get_obj(fb, 0); + struct vkms_gem_object *vkms_obj =3D drm_gem_to_vkms_gem(gem_obj); + + vkms_conn->writeback_vaddr =3D kzalloc(vkms_obj->gem.size, GFP_KERNEL); + + drm_writeback_queue_job(wb_conn, conn_state->writeback_job); + conn_state->writeback_job =3D NULL; + + output->wb_state =3D MW_ONESHOT; +pr_info("VKMS_OBJ (%d): Buff (%d) - Vaddr (%d) - Size =3D %ld - count =3D = %d", vkms_obj !=3D NULL, + vkms_conn->writeback_vaddr !=3D NULL, vkms_obj->vaddr !=3D 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 =3D MW_STOP; + } +} + static const struct drm_connector_helper_funcs vkms_conn_helper_funcs =3D { .get_modes =3D vkms_conn_get_modes, }; =20 +static const struct drm_encoder_funcs vkms_encoder_funcs =3D { + .destroy =3D drm_encoder_cleanup, +}; + +static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = =3D { + .get_modes =3D vkms_conn_get_modes, + .atomic_commit =3D vkms_wb_atomic_commit, +}; + +static int enable_writeback_connector(struct vkms_device *vkmsdev) +{ + struct drm_writeback_connector *wb =3D &vkmsdev->output.wb_connector; + + vkmsdev->output.wb_connector.encoder.possible_crtcs =3D 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 =3D &vkmsdev->output; @@ -77,13 +209,14 @@ int vkms_output_init(struct vkms_device *vkmsdev) goto err_connector_register; } =20 + encoder->possible_crtcs =3D 1; + ret =3D 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 =3D 1; =20 ret =3D drm_connector_attach_encoder(connector, encoder); if (ret) { @@ -91,6 +224,12 @@ int vkms_output_init(struct vkms_device *vkmsdev) goto err_attach; } =20 + if (vkmsdev->output.writeback_enabled) { + ret =3D enable_writeback_connector(vkmsdev); + if (ret) + DRM_ERROR("Failed to init writeback connector\n"); + } + drm_mode_config_reset(dev); =20 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 *v= kmsdev, funcs =3D &vkms_primary_helper_funcs; } =20 - ret =3D drm_universal_plane_init(dev, plane, 0, + ret =3D drm_universal_plane_init(dev, plane, 1, &vkms_plane_funcs, formats, nformats, NULL, type, NULL); --=20 2.21.0 --=20 Rodrigo Siqueira https://siqueira.tech --ddamgngzb27wv7ig Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE4tZ+ii1mjMCMQbfkWJzP/comvP8FAlylCu8ACgkQWJzP/com vP8raBAAzANGjPYdfoDxF8xW0QwQm3Kj48OkETuHyLFUY/F8F7IjS1wRTHjbeDTF C9qy/YHDmFxmt8XscKGfBPVy7TScHCjVgW5NTjPuy2s11LIPBx36O1V4fSAQf13G HU7sEQiYq5qmIgpQTTRtAj0apGtF07dYSbkvuKxrS5Ce/8CWVt8luigpRwoS0a0E OxIMZ+BCSCDXPuMzTcUiQxqKz52l/VXtCSEwdjfAAQ+KXw6KxHeTEo2H8u7O+Q3X o/xy4VmdDF7IpVgWCLQKVpytmBrwY0ircNXp12yvpCSsEowFSGFTJCpCJ+gt8xGz 6z93luPGN4dXGPjje1m5PS3gtwwqAEV1/xE5yDWM3mETvsalOeFVJldHdslvpkvO zHwu7h5aZHhHpFeeUPmT9XZdXhL05DRX3Qsh168Tr2qznu1EOZv2fodHcAcR7U0c qBKgVTrYgOSBaX4CEO8q2X/EoY3TsHjhxxmVJUzJEAiKVWu/vg+Rqz/nZjVt4K3W g0/TLX2SqVvM0A7LEUDyLIOvBKrf9hFtnekmpxZg7N2D54/rqLlOqDqWuUpq7gOy 8ZLgD9xoynIADTupgic/BM6Y4we5EI+7bcHpSyD65zmlzSC/HDdY2kx4kE0G68ZO PhoYKe4Lmb3fq+szUswtcorOC1+B1BmPPFcP3U0YQ0SAQZixs0c= =jlzW -----END PGP SIGNATURE----- --ddamgngzb27wv7ig-- --===============2027743196== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVs --===============2027743196==--