[RFC] drm/vkms: Add writeback support
Rodrigo Siqueira
rodrigosiqueiramelo at gmail.com
Thu Mar 14 15:57:13 UTC 2019
On 03/11, Liviu Dudau wrote:
> On Sun, Mar 10, 2019 at 06:20:34PM -0300, Rodrigo Siqueira wrote:
> > Hi,
>
> Hi Rodrigo,
>
> >
> > In the last few weeks, I was focused on implementing the writeback on
> > VKMS by using the feedback that I got from Brian Starkey and Daniel
> > Vetter in my previous attempt [1]. For testing/guiding my
> > implementation, I’m using a patchset made by Liviu and Brian that adds
> > writeback tests to IGT [2]. This patch, already pass the basic
> > requirements and some of the other tests.
> >
> > So, in this implementation I adopted the following design: you can use
> > writeback_connector, or you can have a virtual connector, but you cannot
> > have both. Should I keep the virtual connector always available and make
> > it possible to add writeback connector as the secondary connector? If
> > so, there's any IGT test for this?
>
> Given that the writeback is one-shot (i.e. no output will be generated
> after the commit that provided a writeback framebuffer), how does that play
> with your userspace? I think that unless you have a specific reason for it,
> having both connectors available all the time makes sense, and the writeback
> connector only gets used by the userspace that sets the right CAPS.
Hi Liviu,
First of all, thank you for the feedback.
Originally, my final goal was to use writeback feature for adding a
visual output for VKMS. I want to make the userspace aware of the
writeback in the VKMS, and use it for displaying things. However, at
this moment, I just want to make sure that VKMS can pass in all of the
tests in the kms_writeback.
Anyway, I agree with your point about keep both connectors enabled. I
will fix it, thanks for shed lights on this issue.
Also, thank you for the given ideas related to the to
'drm_writeback_signal_completion()' problem in my implementation. I will
take a careful look at it; I’m suspecting that I’m making some wrong
assumptions...
Best Regards
Rodrigo Siqueira
> >
> > Secondly, I’m facing problems related to
> > ‘drm_writeback_signal_completion()’, for some reason that I did not know
> > yet I continuously get the following error in a loop:
> >
> > [ +0.000060] CPU: 3 PID: 0 Comm: swapper/3 Tainted: G D W 5.0.0-VKMS #102
> > [ +0.000004] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-20181126_142135-anatol 04/01/2014
> > [ +0.000033] RIP: 0010:drm_writeback_signal_completion+0x10c/0x130 [drm]
> > [ +0.000006] Code: 01 00 00 48 89 43 e8 48 89 43 f0 48 8b 35 3c 67 b3 e4 5b 5d 41 5c 41 5d 41 5e e9 0f 7d aa e3 4c 89 ee 4c 89 e7 e8 a4 4e 18 e4 <0f> 0b 5b 5d 41 5c 41 5d 41 5e c3 e8 54 2f f8 e3 eb 9a 0f 0b e9 60
> > [ +0.000004] RSP: 0018:ffff98593cb83ec0 EFLAGS: 00010082
> > [ +0.000006] RAX: 0000000080010002 RBX: ffff98593bbb54b0 RCX: 0000000000000000
> > [ +0.000005] RDX: 0000000000000001 RSI: 0000000000000082 RDI: 00000000ffffffff
> > [ +0.000004] RBP: ffff98593bbb54b0 R08: 0000000000000000 R09: ffff98593cb83e20
> > [ +0.000004] R10: ffff98593a07d600 R11: 0000000000000000 R12: ffff98593bbb54a8
> > [ +0.000004] R13: 0000000000000082 R14: 0000000000000000 R15: ffff98593cb9cd38
> > [ +0.000006] FS: 0000000000000000(0000) GS:ffff98593cb80000(0000) knlGS:0000000000000000
> > [ +0.000004] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [ +0.000005] CR2: 00007f3cea2e8000 CR3: 00000000ba6ec000 CR4: 00000000000006e0
> > [ +0.000010] Call Trace:
> > [ +0.000007] <IRQ>
> > [ +0.000013] ? vkms_disable_vblank+0x20/0x20 [vkms]
> > [ +0.000008] vkms_vblank_simulate+0xef/0x110 [vkms]
> > [ +0.000008] ? vkms_disable_vblank+0x20/0x20 [vkms]
> > [ +0.000007] __hrtimer_run_queues+0x100/0x2d0
> > [ +0.000008] hrtimer_interrupt+0x10a/0x220
> > [ +0.000008] ? kvm_sched_clock_read+0x5/0x10
> > [ +0.000010] smp_apic_timer_interrupt+0x69/0x160
> > [ +0.000007] apic_timer_interrupt+0xf/0x20
> > [ +0.000004] </IRQ>
> > [ +0.000008] RIP: 0010:native_safe_halt+0x2/0x10
> > [ +0.000005] Code: 5b ff ff ff 7f 5b c3 65 48 8b 04 25 00 5c 01 00 f0 80 48 02 20 48 8b 00 a8 08 75 c3 eb 8b 90 90 90 90 90 90 90 90 90 90 fb f4 <c3> 66 66 2e 0f 1f 84 00 00 00 00 00 66 90 f4 c3 90 90 90 90 90 90
> > [ +0.000005] RSP: 0018:ffffb937c06b3eb8 EFLAGS: 00000202 ORIG_RAX: ffffffffffffff13
> > [ +0.000005] RAX: 0000000000000003 RBX: 0000000000000003 RCX: 0000000000000001
> > [ +0.000004] RDX: 0000000000000001 RSI: ffffffffa4e6da3e RDI: ffffffffa4e6dd30
> > [ +0.000005] RBP: ffffffffa51252e0 R08: ffffffffa55e7438 R09: 0000000000000000
> > [ +0.000003] R10: 0000000000000000 R11: ffffffffa5049fb8 R12: 0000000000000000
> > [ +0.000004] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> > [ +0.000012] default_idle+0x1a/0x170
> > [ +0.000010] do_idle+0x1d5/0x250
> > [ +0.000007] cpu_startup_entry+0x19/0x20
> > [ +0.000007] start_secondary+0x1aa/0x200
> > [ +0.000008] secondary_startup_64+0xa4/0xb0
> > [ +0.000008] ---[ end trace ec12666e4ecb96f7 ]---
> >
> > Any ideas? Or any comments?
>
> What line does the "drm_writeback_signal_completion+0x10c/0x130" address
> resolves to? Also, am I reading correctly that vkms_disable_vblank()
> gets called (twice) in the IRQ handler? Does that make sense?
>
> The only thing that I can suggest at the moment is to have a look in your
> code and check if there are any assumptions regarding vblank interrupts
> being generated for writeback connectors. Usually the hardware that supports
> writeback functionality is only going to generate an userspace event at
> the end of the writeback so you might have to use the drm_atomic_helper_fake_vblank()
> function for the rest of the time.
>
> Best regards,
> Liviu
>
> >
> > Thanks
> > Best Regards
> >
> > 1. https://patchwork.freedesktop.org/patch/257771/?series=51325&rev=1
> > 2. https://patchwork.freedesktop.org/series/39229/
> >
> > ---
> > drivers/gpu/drm/vkms/vkms_crtc.c | 3 +
> > drivers/gpu/drm/vkms/vkms_drv.c | 8 ++
> > drivers/gpu/drm/vkms/vkms_drv.h | 21 ++++
> > drivers/gpu/drm/vkms/vkms_output.c | 176 ++++++++++++++++++++++++++---
> > drivers/gpu/drm/vkms/vkms_plane.c | 2 +-
> > 5 files changed, 195 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > index 8a9aeb0a9ea8..d9948208e8eb 100644
> > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > @@ -19,6 +19,9 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> > if (!ret)
> > DRM_ERROR("vkms failure on handling vblank");
> >
> > + if (output->writeback_enabled)
> > + drm_writeback_signal_completion(&output->mw_connector, 0);
> > +
> > 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..2c26fe686b93 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
> > @@ -46,6 +47,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 +75,12 @@ struct vkms_output {
> > struct drm_crtc crtc;
> > struct drm_encoder encoder;
> > struct drm_connector connector;
> > + struct drm_writeback_connector mw_connector;
> > struct hrtimer vblank_hrtimer;
> > ktime_t period_ns;
> > struct drm_pending_vblank_event *event;
> > bool crc_enabled;
> > + int writeback_enabled;
> > /* ordered wq for crc_work */
> > struct workqueue_struct *crc_workq;
> > /* protects concurrent access to crc_data */
> > @@ -93,6 +106,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, mw_connector)
> > +
> > #define drm_device_to_vkms_device(target) \
> > container_of(target, struct vkms_device, drm)
> >
> > @@ -105,6 +124,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_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> > index 3b162b25312e..0e55e7299d2e 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,14 +12,92 @@ static void vkms_connector_destroy(struct drm_connector *connector)
> > drm_connector_cleanup(connector);
> > }
> >
> > +static void vkms_connector_reset(struct drm_connector *connector)
> > +{
> > + struct vkms_connector_state *mw_state;
> > + struct drm_writeback_connector *wb = drm_connector_to_wb_connector(connector);
> > + struct vkms_output *output = drm_wb_to_vkms_output(wb);
> > +
> > + if (!output->writeback_enabled)
> > + return drm_atomic_helper_connector_reset(connector);
> > +
> > + mw_state = kzalloc(sizeof(*mw_state), GFP_KERNEL);
> > + if (!mw_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, &mw_state->base);
> > +}
> > +
> > +static struct drm_connector_state *
> > +vkms_connector_duplicate_state(struct drm_connector *connector)
> > +{
> > + struct vkms_connector_state *mw_state, *mw_current_state;
> > + struct drm_writeback_connector *wb = drm_connector_to_wb_connector(connector);
> > + struct vkms_output *output = drm_wb_to_vkms_output(wb);
> > +
> > + if (!output->writeback_enabled)
> > + return drm_atomic_helper_connector_duplicate_state(connector);
> > +
> > + if (WARN_ON(!connector->state))
> > + return NULL;
> > +
> > + mw_state = kzalloc(sizeof(*mw_state), GFP_KERNEL);
> > + if (!mw_state)
> > + return NULL;
> > +
> > + mw_current_state = to_wb_state(connector->state);
> > + mw_state->writeback_vaddr = mw_current_state->writeback_vaddr;
> > +
> > + __drm_atomic_helper_connector_duplicate_state(connector, &mw_state->base);
> > +
> > + return &mw_state->base;
> > +}
> > +
> > static const struct drm_connector_funcs vkms_connector_funcs = {
> > .fill_modes = drm_helper_probe_single_connector_modes,
> > .destroy = vkms_connector_destroy,
> > - .reset = drm_atomic_helper_connector_reset,
> > - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > + .reset = vkms_connector_reset,
> > + .atomic_duplicate_state = vkms_connector_duplicate_state,
> > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > };
> >
> > +static int vkms_mw_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;
> > + }
> > +
> > + // TODO: Do we need to validate each plane?
> > +
> > + return 0;
> > +}
> > +
> > static const struct drm_encoder_funcs vkms_encoder_funcs = {
> > .destroy = drm_encoder_cleanup,
> > };
> > @@ -32,8 +112,57 @@ static int vkms_conn_get_modes(struct drm_connector *connector)
> > return count;
> > }
> >
> > +static void vkms_mw_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 *mw_conn = &output->mw_connector;
> > +
> > + struct drm_connector_state *conn_state = mw_conn->base.state;
> > +
> > + struct vkms_connector_state *vkms_conn = to_wb_state(conn_state);
> > + struct vkms_crc_data *primary_plane = NULL;
> > + struct drm_plane *plane;
> > +
> > + 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(mw_conn, conn_state->writeback_job);
> > + conn_state->writeback_job = NULL;
> > +
> > + drm_for_each_plane(plane, conn->dev) {
> > + struct vkms_plane_state *vplane_state;
> > + struct vkms_crc_data *crc_data;
> > +
> > + vplane_state = to_vkms_plane_state(plane->state);
> > + crc_data = vplane_state->crc_data;
> > +
> > + if (drm_framebuffer_read_refcount(&crc_data->fb) == 0)
> > + continue;
> > +
> > + if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> > + primary_plane = crc_data;
> > + }
> > +
> > + memcpy(vkms_conn->writeback_vaddr, vkms_obj->vaddr, vkms_obj->gem.size);
> > + }
> > +}
> > +
> > static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
> > .get_modes = vkms_conn_get_modes,
> > + .atomic_commit = vkms_mw_atomic_commit,
> > +};
> > +
> > +static const struct drm_encoder_helper_funcs vkms_encoder_helper_funcs = {
> > + .atomic_check = vkms_mw_encoder_atomic_check,
> > };
> >
> > int vkms_output_init(struct vkms_device *vkmsdev)
> > @@ -62,8 +191,24 @@ int vkms_output_init(struct vkms_device *vkmsdev)
> > if (ret)
> > goto err_crtc;
> >
> > - ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
> > - DRM_MODE_CONNECTOR_VIRTUAL);
> > + if (vkmsdev->output.writeback_enabled) {
> > + const u32 *formats = vkms_formats;
> > + int n_formats = ARRAY_SIZE(vkms_formats);
> > + struct drm_writeback_connector *wb_connector;
> > +
> > + vkmsdev->output.mw_connector.encoder.possible_crtcs = 1;
> > +
> > + wb_connector = &vkmsdev->output.mw_connector;
> > + connector = &wb_connector->base;
> > + ret = drm_writeback_connector_init(dev, wb_connector,
> > + &vkms_connector_funcs,
> > + &vkms_encoder_helper_funcs,
> > + formats, n_formats);
> > + } else {
> > + ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
> > + DRM_MODE_CONNECTOR_VIRTUAL);
> > + }
> > +
> > if (ret) {
> > DRM_ERROR("Failed to init connector\n");
> > goto err_connector;
> > @@ -77,18 +222,21 @@ int vkms_output_init(struct vkms_device *vkmsdev)
> > goto err_connector_register;
> > }
> >
> > - 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) {
> > - DRM_ERROR("Failed to attach connector to encoder\n");
> > - goto err_attach;
> > + if (!vkmsdev->output.writeback_enabled) {
> > + 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;
> > + }
> > +
> > + ret = drm_connector_attach_encoder(connector, encoder);
> > + if (ret) {
> > + DRM_ERROR("Failed to attach connector to encoder\n");
> > + goto err_attach;
> > + }
> > }
> >
> > drm_mode_config_reset(dev);
> > 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
> > Graduate Student
> > Department of Computer Science
> > University of São Paulo
>
> --
> ====================
> | I would like to |
> | fix the world, |
> | but they're not |
> | giving me the |
> \ source code! /
> ---------------
> ¯\_(ツ)_/¯
--
Rodrigo Siqueira
https://siqueira.tech
Graduate Student
Department of Computer Science
University of São Paulo
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20190314/06708de4/attachment-0001.sig>
More information about the dri-devel
mailing list