On 10/24, Brian Starkey wrote: > Hi Rodrigo, Hi Brian, Thanks a lot for your feedback! I learned a lot with all of them :) I will work for make a real PATCH soon. Again, thanks a lot. Best Regards > 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 -- 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
