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

[Daniel Vetter <daniel@ffwll.ch>]

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?

Yeah separate connector is imo better, with module config knob. Eventually
we can move that into configfs or similar.

> 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?

Yeah, you don't need anything fancy in the drm_encoder. I think you could
reuse the same dummy drm_encoder as we use for the normal connector.

> 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?

Yeah, writeback connector doesn't work like a normal connector, so it's
hidden by default. There should be special writeback connector tests in
igt though, but I guess those haven't been merged yet. Ask Liviu and Brian
from arm where they're stuck. Without those it's going to be rather hard
to make this work correctly.

Cheers, Daniel

> 
> Thanks!
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> ---
>  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);
>  	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,
>  };
>  
> @@ -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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel