Re: [RFC] drm/vkms: Add writeback support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux