Hi Rodrigo, On Mon, Oct 22, 2018 at 09:27:18AM -0300, Rodrigo Siqueira wrote: >Hi, > >I started to work for add the writeback feature into the VKMS, and this >RFC represents my first draft. As a result, I have some questions which >I list below. > >1. In this patch, I replaced the virtual connector by the writeback >connector. Is it a good idea? Or should I keep both connectors? If I >keep both connectors, should I add a module parameter to enable the >writeback? > >2. When I added the writeback connector (in >drm_writeback_connector_init()), I noticed that I had to indicate the >struct drm_encoder_helper_funcs. However, I think that VKMS does not >need anything sophisticated related to encoders at the writeback >perspective. Am I right? Or did I missed something? > >3. I just discovered that the writeback connectors are not exposed as a >connector for the userspace, as a result, IGT tests do not work and >Wayland is unable to run as well. Simon Ser and Daniel Stone, explained >to me that I have to update the compositor (in my case Weston) to set >DRM_CLIENT_CAP_WRITEBACK_CONNECTORS via drmSetClientCap(). Since I do >not have experience with userspace, anyone can point me out another >aspect that I have to take a look to make the writeback work in the user >space? Also, Is it possible to update the IGT test or do I need to >create a new set of tests? Alex added support for writeback connectors into drm_hwcomposer - you can see that here[1] There's also some igt tests like Daniel mentioned - I think Liviu already pointed you at those. Some more review below, Cheers! -Brian [1] https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/merge_requests/3 > >Thanks! > >Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@xxxxxxxxx> >--- > drivers/gpu/drm/vkms/vkms_crc.c | 9 ++- > drivers/gpu/drm/vkms/vkms_crtc.c | 2 + > drivers/gpu/drm/vkms/vkms_drv.h | 3 +- > drivers/gpu/drm/vkms/vkms_output.c | 107 +++++++++++++++++++++-------- > 4 files changed, 89 insertions(+), 32 deletions(-) > >diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c >index 9d9e8146db90..5d9ac45e00f2 100644 >--- a/drivers/gpu/drm/vkms/vkms_crc.c >+++ b/drivers/gpu/drm/vkms/vkms_crc.c >@@ -109,7 +109,8 @@ static void compose_cursor(struct vkms_crc_data *cursor_crc, > } > > static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc, >- struct vkms_crc_data *cursor_crc) >+ struct vkms_crc_data *cursor_crc, >+ struct vkms_output *out) > { > struct drm_framebuffer *fb = &primary_crc->fb; > struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0); >@@ -130,6 +131,10 @@ static uint32_t _vkms_get_crc(struct vkms_crc_data *primary_crc, > } > > memcpy(vaddr_out, vkms_obj->vaddr, vkms_obj->gem.size); >+ // TODO: Remove this copy from crc, and make this sort of operation >+ // generic since we can use in different places >+ memcpy(out->connector.base.state->writeback_job->fb, >+ vkms_obj->vaddr, vkms_obj->gem.size); This looks like it would crash terribly, you're copying the pixel data (from vkms_obj->vaddr) into a "struct drm_framebuffer *" which is a pointer to the struct, rather than somewhere to put the actual pixels. The same way you get the vaddr for the input framebuffer, you need to get the vaddr for the output framebuffer too - via drm_gem_fb_get_obj() and then converting that to a vkms_gem_object. Then use that vaddr as the destination of the memcpy. However, see my comments at the bottom about drm_writeback_queue_job(). You might be better getting and storing the vaddr during atomic_commit(), rather than in this function. In order for this to be a straight memcpy, you need to make sure that the input and output buffers have the same size, pixel format, same stride etc. as well so you'll need to check that in atomic_check(). Once it's working you can relax those restrictions by copying a line at a time or doing pixel format conversion - but I don't think it's necessary to start with. There might be an opportunity to skip a copy here too - if you have a framebuffer attached to the writeback connector, you can skip allocating "vaddr_out" and just compose the cursor directly into the output framebuffer, and calculate the CRC on that. That's definitely an optimisation rather than a requirement though. > mutex_unlock(&vkms_obj->pages_lock); > > if (cursor_crc) >@@ -193,7 +198,7 @@ void vkms_crc_work_handle(struct work_struct *work) > } > > if (primary_crc) >- crc32 = _vkms_get_crc(primary_crc, cursor_crc); >+ crc32 = _vkms_get_crc(primary_crc, cursor_crc, out); > > frame_end = drm_crtc_accurate_vblank_count(crtc); > >diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c >index 177bbcb38306..d33ddcc40a29 100644 >--- a/drivers/gpu/drm/vkms/vkms_crtc.c >+++ b/drivers/gpu/drm/vkms/vkms_crtc.c >@@ -48,6 +48,8 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) > > _vblank_handle(output); > >+ drm_writeback_signal_completion(&output->connector, 0); >+ > ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer, > output->period_ns); > >diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h >index 1c93990693e3..1e8184ad872f 100644 >--- a/drivers/gpu/drm/vkms/vkms_drv.h >+++ b/drivers/gpu/drm/vkms/vkms_drv.h >@@ -5,6 +5,7 @@ > #include <drm/drm.h> > #include <drm/drm_gem.h> > #include <drm/drm_encoder.h> >+#include <drm/drm_writeback.h> > #include <linux/hrtimer.h> > > #define XRES_MIN 20 >@@ -61,7 +62,7 @@ struct vkms_crtc_state { > struct vkms_output { > struct drm_crtc crtc; > struct drm_encoder encoder; >- struct drm_connector connector; >+ struct drm_writeback_connector connector; > struct hrtimer vblank_hrtimer; > ktime_t period_ns; > struct drm_pending_vblank_event *event; >diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c >index 271a0eb9042c..46622d012107 100644 >--- a/drivers/gpu/drm/vkms/vkms_output.c >+++ b/drivers/gpu/drm/vkms/vkms_output.c >@@ -9,6 +9,13 @@ > #include "vkms_drv.h" > #include <drm/drm_crtc_helper.h> > #include <drm/drm_atomic_helper.h> >+#include <drm/drm_writeback.h> >+ >+#define to_wb_state(_state) (struct vkms_writeback_connector_state *)(_state) >+ >+struct vkms_writeback_connector_state { >+ struct drm_connector_state base; >+}; > > static void vkms_connector_destroy(struct drm_connector *connector) > { >@@ -16,11 +23,48 @@ static void vkms_connector_destroy(struct drm_connector *connector) > drm_connector_cleanup(connector); > } > >+static void vkms_connector_reset(struct drm_connector *connector) >+{ >+ struct vkms_writeback_connector_state *wb_state = >+ kzalloc(sizeof(*wb_state), GFP_KERNEL); >+ >+ 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_connector_duplicate_state(struct drm_connector *connector) >+{ >+ struct vkms_writeback_connector_state *wb_state; >+ >+ if (WARN_ON(!connector->state)) >+ return NULL; >+ >+ wb_state = kzalloc(sizeof(*wb_state), GFP_KERNEL); >+ if (!wb_state) >+ return NULL; >+ >+ __drm_atomic_helper_connector_duplicate_state(connector, >+ &wb_state->base); >+ >+ return &wb_state->base; >+} >+ >+static enum drm_connector_status >+vkms_connector_detect(struct drm_connector *connector, bool force) >+{ >+ return connector_status_connected; >+} >+ > static const struct drm_connector_funcs vkms_connector_funcs = { >+ .reset = vkms_connector_reset, >+ .detect = vkms_connector_detect, > .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, >+ .atomic_duplicate_state = vkms_connector_duplicate_state, > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > }; You're missing the call to drm_writeback_queue_job(), which is required. Your connector needs to have an atomic_commit callback in its drm_connector_helper_funcs, and in there you need to "queue" the job, something like: struct drm_writeback_connector *wb_conn = &out->connector; drm_writeback_queue_job(wb_conn, wb_conn->base.state->writeback_job); wb_conn->base.state->writeback_job = NULL; Because you need to set the job pointer to NULL there, before that you'll need to stash the destination somewhere else, to be used as the destination for the memcpy() in _vkms_get_crc(). The simplest thing to do is probably to just add a new void *writeback_vaddr to vkms_writeback_connector_state, and set it to the output framebuffer vaddr before you call _queue_job(). The core will make sure the framebuffer is properly refcounted until you call drm_writeback_signal_completion(), so as long as you only call that after you've finished copying the data, you're safe. In a "real" HW device, we write that pointer directly to a HW register, so we don't need to track it in our connector state. But you don't have a HW register to put it in, so you'll need to track it yourself :-) > >@@ -42,12 +86,35 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = { > .get_modes = vkms_conn_get_modes, > }; > >+static int vkms_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; >+ } >+ >+ return 0; >+} >+ >+static const struct drm_encoder_helper_funcs vkms_encoder_helper_funcs = { >+ .atomic_check = vkms_encoder_atomic_check, >+}; >+ > int vkms_output_init(struct vkms_device *vkmsdev) > { > struct vkms_output *output = &vkmsdev->output; > struct drm_device *dev = &vkmsdev->drm; >- struct drm_connector *connector = &output->connector; >- struct drm_encoder *encoder = &output->encoder; >+ struct drm_writeback_connector *connector = &output->connector; > struct drm_crtc *crtc = &output->crtc; > struct drm_plane *primary, *cursor = NULL; > int ret; >@@ -68,47 +135,29 @@ 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); >+ ret = drm_writeback_connector_init(dev, connector, >+ &vkms_connector_funcs, >+ &vkms_encoder_helper_funcs, >+ vkms_formats, 1); > if (ret) { > DRM_ERROR("Failed to init connector\n"); > goto err_connector; > } > >- drm_connector_helper_add(connector, &vkms_conn_helper_funcs); >+ drm_connector_helper_add(&connector->base, &vkms_conn_helper_funcs); > >- ret = drm_connector_register(connector); >+ ret = drm_connector_register(&connector->base); > if (ret) { > DRM_ERROR("Failed to register connector\n"); > 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; >- } >- > drm_mode_config_reset(dev); > > return 0; > >-err_attach: >- drm_encoder_cleanup(encoder); >- >-err_encoder: >- drm_connector_unregister(connector); >- > err_connector_register: >- drm_connector_cleanup(connector); >+ drm_connector_cleanup(&connector->base); > > err_connector: > drm_crtc_cleanup(crtc); >-- >2.19.1 > > >-- >Rodrigo Siqueira >https://siqueira.tech >https://twitter.com/siqueirajordao >Graduate Student >Department of Computer Science >University of São Paulo _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
