Re: [PATCH 7/9] drm/xen-front: Implement KMS/connector handling

[Oleksandr Andrushchenko <andr2000@xxxxxxxxx>]

On 03/05/2018 11:23 AM, Daniel Vetter wrote:
> On Wed, Feb 21, 2018 at 10:03:40AM +0200, Oleksandr Andrushchenko wrote:
> > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> >
> > Implement kernel modesetiing/connector handling using
> > DRM simple KMS helper pipeline:
> >
> > - implement KMS part of the driver with the help of DRM
> >    simple pipepline helper which is possible due to the fact
> >    that the para-virtualized driver only supports a single
> >    (primary) plane:
> >    - initialize connectors according to XenStore configuration
> >    - handle frame done events from the backend
> >    - generate vblank events
> >    - create and destroy frame buffers and propagate those
> >      to the backend
> >    - propagate set/reset mode configuration to the backend on display
> >      enable/disable callbacks
> >    - send page flip request to the backend and implement logic for
> >      reporting backend IO errors on prepare fb callback
> >
> > - implement virtual connector handling:
> >    - support only pixel formats suitable for single plane modes
> >    - make sure the connector is always connected
> >    - support a single video mode as per para-virtualized driver
> >      configuration
> >
> > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> I think once you've removed the midlayer in the previous patch it would
> makes sense to merge the 2 patches into 1.
ok, will squash the two
> Bunch more comments below.
> -Daniel
>
> > ---
> >   drivers/gpu/drm/xen/Makefile             |   2 +
> >   drivers/gpu/drm/xen/xen_drm_front_conn.c | 125 +++++++++++++
> >   drivers/gpu/drm/xen/xen_drm_front_conn.h |  35 ++++
> >   drivers/gpu/drm/xen/xen_drm_front_drv.c  |  15 ++
> >   drivers/gpu/drm/xen/xen_drm_front_drv.h  |  12 ++
> >   drivers/gpu/drm/xen/xen_drm_front_kms.c  | 299 +++++++++++++++++++++++++++++++
> >   drivers/gpu/drm/xen/xen_drm_front_kms.h  |  30 ++++
> >   7 files changed, 518 insertions(+)
> >   create mode 100644 drivers/gpu/drm/xen/xen_drm_front_conn.c
> >   create mode 100644 drivers/gpu/drm/xen/xen_drm_front_conn.h
> >   create mode 100644 drivers/gpu/drm/xen/xen_drm_front_kms.c
> >   create mode 100644 drivers/gpu/drm/xen/xen_drm_front_kms.h
> >
> > diff --git a/drivers/gpu/drm/xen/Makefile b/drivers/gpu/drm/xen/Makefile
> > index d3068202590f..4fcb0da1a9c5 100644
> > --- a/drivers/gpu/drm/xen/Makefile
> > +++ b/drivers/gpu/drm/xen/Makefile
> > @@ -2,6 +2,8 @@
> >   
> >   drm_xen_front-objs := xen_drm_front.o \
> >   		      xen_drm_front_drv.o \
> > +		      xen_drm_front_kms.o \
> > +		      xen_drm_front_conn.o \
> >   		      xen_drm_front_evtchnl.o \
> >   		      xen_drm_front_shbuf.o \
> >   		      xen_drm_front_cfg.o
> > diff --git a/drivers/gpu/drm/xen/xen_drm_front_conn.c b/drivers/gpu/drm/xen/xen_drm_front_conn.c
> > new file mode 100644
> > index 000000000000..d9986a2e1a3b
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xen/xen_drm_front_conn.c
> > @@ -0,0 +1,125 @@
> > +/*
> > + *  Xen para-virtual DRM device
> > + *
> > + *   This program is free software; you can redistribute it and/or modify
> > + *   it under the terms of the GNU General Public License as published by
> > + *   the Free Software Foundation; either version 2 of the License, or
> > + *   (at your option) any later version.
> > + *
> > + *   This program is distributed in the hope that it will be useful,
> > + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + *   GNU General Public License for more details.
> > + *
> > + * Copyright (C) 2016-2018 EPAM Systems Inc.
> > + *
> > + * Author: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> > + */
> > +
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_crtc_helper.h>
> > +
> > +#include <video/videomode.h>
> > +
> > +#include "xen_drm_front_conn.h"
> > +#include "xen_drm_front_drv.h"
> > +
> > +static struct xen_drm_front_drm_pipeline *
> > +to_xen_drm_pipeline(struct drm_connector *connector)
> > +{
> > +	return container_of(connector, struct xen_drm_front_drm_pipeline, conn);
> > +}
> > +
> > +static const uint32_t plane_formats[] = {
> > +	DRM_FORMAT_RGB565,
> > +	DRM_FORMAT_RGB888,
> > +	DRM_FORMAT_XRGB8888,
> > +	DRM_FORMAT_ARGB8888,
> > +	DRM_FORMAT_XRGB4444,
> > +	DRM_FORMAT_ARGB4444,
> > +	DRM_FORMAT_XRGB1555,
> > +	DRM_FORMAT_ARGB1555,
> > +};
> > +
> > +const uint32_t *xen_drm_front_conn_get_formats(int *format_count)
> > +{
> > +	*format_count = ARRAY_SIZE(plane_formats);
> > +	return plane_formats;
> > +}
> > +
> > +static enum drm_connector_status connector_detect(
> > +		struct drm_connector *connector, bool force)
> > +{
> > +	if (drm_dev_is_unplugged(connector->dev))
> > +		return connector_status_disconnected;
> > +
> > +	return connector_status_connected;
> > +}
> > +
> > +#define XEN_DRM_NUM_VIDEO_MODES		1
> > +#define XEN_DRM_CRTC_VREFRESH_HZ	60
> > +
> > +static int connector_get_modes(struct drm_connector *connector)
> > +{
> > +	struct xen_drm_front_drm_pipeline *pipeline =
> > +		to_xen_drm_pipeline(connector);
> > +	struct drm_display_mode *mode;
> > +	struct videomode videomode;
> > +	int width, height;
> > +
> > +	mode = drm_mode_create(connector->dev);
> > +	if (!mode)
> > +		return 0;
> > +
> > +	memset(&videomode, 0, sizeof(videomode));
> > +	videomode.hactive = pipeline->width;
> > +	videomode.vactive = pipeline->height;
> > +	width = videomode.hactive + videomode.hfront_porch +
> > +		videomode.hback_porch + videomode.hsync_len;
> > +	height = videomode.vactive + videomode.vfront_porch +
> > +		videomode.vback_porch + videomode.vsync_len;
> > +	videomode.pixelclock = width * height * XEN_DRM_CRTC_VREFRESH_HZ;
> > +	mode->type = DRM_MODE_TYPE_PREFERRED | DRM_MODE_TYPE_DRIVER;
> > +
> > +	drm_display_mode_from_videomode(&videomode, mode);
> > +	drm_mode_probed_add(connector, mode);
> > +	return XEN_DRM_NUM_VIDEO_MODES;
> > +}
> > +
> > +static int connector_mode_valid(struct drm_connector *connector,
> > +		struct drm_display_mode *mode)
> > +{
> > +	struct xen_drm_front_drm_pipeline *pipeline =
> > +			to_xen_drm_pipeline(connector);
> > +
> > +	if (mode->hdisplay != pipeline->width)
> > +		return MODE_ERROR;
> > +
> > +	if (mode->vdisplay != pipeline->height)
> > +		return MODE_ERROR;
> > +
> > +	return MODE_OK;
> > +}
> > +
> > +static const struct drm_connector_helper_funcs connector_helper_funcs = {
> > +	.get_modes = connector_get_modes,
> > +	.mode_valid = connector_mode_valid,
> > +};
> > +
> > +static const struct drm_connector_funcs connector_funcs = {
> > +	.detect = connector_detect,
> > +	.fill_modes = drm_helper_probe_single_connector_modes,
> > +	.destroy = drm_connector_cleanup,
> > +	.reset = drm_atomic_helper_connector_reset,
> > +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > +};
> > +
> > +int xen_drm_front_conn_init(struct xen_drm_front_drm_info *drm_info,
> > +		struct drm_connector *connector)
> > +{
> > +	drm_connector_helper_add(connector, &connector_helper_funcs);
> > +
> > +	return drm_connector_init(drm_info->drm_dev, connector,
> > +		&connector_funcs, DRM_MODE_CONNECTOR_VIRTUAL);
> > +}
> > diff --git a/drivers/gpu/drm/xen/xen_drm_front_conn.h b/drivers/gpu/drm/xen/xen_drm_front_conn.h
> > new file mode 100644
> > index 000000000000..708e80d45985
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xen/xen_drm_front_conn.h
> > @@ -0,0 +1,35 @@
> > +/*
> > + *  Xen para-virtual DRM device
> > + *
> > + *   This program is free software; you can redistribute it and/or modify
> > + *   it under the terms of the GNU General Public License as published by
> > + *   the Free Software Foundation; either version 2 of the License, or
> > + *   (at your option) any later version.
> > + *
> > + *   This program is distributed in the hope that it will be useful,
> > + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + *   GNU General Public License for more details.
> > + *
> > + * Copyright (C) 2016-2018 EPAM Systems Inc.
> > + *
> > + * Author: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> > + */
> > +
> > +#ifndef __XEN_DRM_FRONT_CONN_H_
> > +#define __XEN_DRM_FRONT_CONN_H_
> > +
> > +#include <drm/drmP.h>
> > +#include <drm/drm_crtc.h>
> > +#include <drm/drm_encoder.h>
> > +
> > +#include <linux/wait.h>
> > +
> > +struct xen_drm_front_drm_info;
> > +
> > +const uint32_t *xen_drm_front_conn_get_formats(int *format_count);
> > +
> > +int xen_drm_front_conn_init(struct xen_drm_front_drm_info *drm_info,
> > +		struct drm_connector *connector);
> > +
> > +#endif /* __XEN_DRM_FRONT_CONN_H_ */
> > diff --git a/drivers/gpu/drm/xen/xen_drm_front_drv.c b/drivers/gpu/drm/xen/xen_drm_front_drv.c
> > index b3764d5ed0f6..e8862d26ba27 100644
> > --- a/drivers/gpu/drm/xen/xen_drm_front_drv.c
> > +++ b/drivers/gpu/drm/xen/xen_drm_front_drv.c
> > @@ -23,6 +23,7 @@
> >   #include "xen_drm_front.h"
> >   #include "xen_drm_front_cfg.h"
> >   #include "xen_drm_front_drv.h"
> > +#include "xen_drm_front_kms.h"
> >   
> >   static int dumb_create(struct drm_file *filp,
> >   		struct drm_device *dev, struct drm_mode_create_dumb *args)
> > @@ -41,6 +42,13 @@ static void free_object(struct drm_gem_object *obj)
> >   static void on_frame_done(struct platform_device *pdev,
> >   		int conn_idx, uint64_t fb_cookie)
> >   {
> > +	struct xen_drm_front_drm_info *drm_info = platform_get_drvdata(pdev);
> > +
> > +	if (unlikely(conn_idx >= drm_info->cfg->num_connectors))
> > +		return;
> > +
> > +	xen_drm_front_kms_on_frame_done(&drm_info->pipeline[conn_idx],
> > +			fb_cookie);
> >   }
> >   
> >   static void lastclose(struct drm_device *dev)
> > @@ -157,6 +165,12 @@ int xen_drm_front_drv_probe(struct platform_device *pdev,
> >   		return ret;
> >   	}
> >   
> > +	ret = xen_drm_front_kms_init(drm_info);
> > +	if (ret) {
> > +		DRM_ERROR("Failed to initialize DRM/KMS, ret %d\n", ret);
> > +		goto fail_modeset;
> > +	}
> > +
> >   	dev->irq_enabled = 1;
> >   
> >   	ret = drm_dev_register(dev, 0);
> > @@ -172,6 +186,7 @@ int xen_drm_front_drv_probe(struct platform_device *pdev,
> >   
> >   fail_register:
> >   	drm_dev_unregister(dev);
> > +fail_modeset:
> >   	drm_mode_config_cleanup(dev);
> >   	return ret;
> >   }
> > diff --git a/drivers/gpu/drm/xen/xen_drm_front_drv.h b/drivers/gpu/drm/xen/xen_drm_front_drv.h
> > index aaa476535c13..563318b19f34 100644
> > --- a/drivers/gpu/drm/xen/xen_drm_front_drv.h
> > +++ b/drivers/gpu/drm/xen/xen_drm_front_drv.h
> > @@ -20,14 +20,24 @@
> >   #define __XEN_DRM_FRONT_DRV_H_
> >   
> >   #include <drm/drmP.h>
> > +#include <drm/drm_simple_kms_helper.h>
> >   
> >   #include "xen_drm_front.h"
> >   #include "xen_drm_front_cfg.h"
> > +#include "xen_drm_front_conn.h"
> >   
> >   struct xen_drm_front_drm_pipeline {
> >   	struct xen_drm_front_drm_info *drm_info;
> >   
> >   	int index;
> > +
> > +	struct drm_simple_display_pipe pipe;
> > +
> > +	struct drm_connector conn;
> > +	/* these are only for connector mode checking */
> > +	int width, height;
> > +	/* last backend error seen on page flip */
> > +	int pgflip_last_error;
> >   };
> >   
> >   struct xen_drm_front_drm_info {
> > @@ -35,6 +45,8 @@ struct xen_drm_front_drm_info {
> >   	struct xen_drm_front_ops *front_ops;
> >   	struct drm_device *drm_dev;
> >   	struct xen_drm_front_cfg *cfg;
> > +
> > +	struct xen_drm_front_drm_pipeline pipeline[XEN_DRM_FRONT_MAX_CRTCS];
> >   };
> >   
> >   static inline uint64_t xen_drm_front_fb_to_cookie(
> > diff --git a/drivers/gpu/drm/xen/xen_drm_front_kms.c b/drivers/gpu/drm/xen/xen_drm_front_kms.c
> > new file mode 100644
> > index 000000000000..ad94c28835cd
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xen/xen_drm_front_kms.c
> > @@ -0,0 +1,299 @@
> > +/*
> > + *  Xen para-virtual DRM device
> > + *
> > + *   This program is free software; you can redistribute it and/or modify
> > + *   it under the terms of the GNU General Public License as published by
> > + *   the Free Software Foundation; either version 2 of the License, or
> > + *   (at your option) any later version.
> > + *
> > + *   This program is distributed in the hope that it will be useful,
> > + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + *   GNU General Public License for more details.
> > + *
> > + * Copyright (C) 2016-2018 EPAM Systems Inc.
> > + *
> > + * Author: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> > + */
> > +
> > +#include "xen_drm_front_kms.h"
> > +
> > +#include <drm/drmP.h>
> > +#include <drm/drm_atomic.h>
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_gem.h>
> > +#include <drm/drm_gem_framebuffer_helper.h>
> > +
> > +#include "xen_drm_front.h"
> > +#include "xen_drm_front_conn.h"
> > +#include "xen_drm_front_drv.h"
> > +
> > +static struct xen_drm_front_drm_pipeline *
> > +to_xen_drm_pipeline(struct drm_simple_display_pipe *pipe)
> > +{
> > +	return container_of(pipe, struct xen_drm_front_drm_pipeline, pipe);
> > +}
> > +
> > +static void fb_destroy(struct drm_framebuffer *fb)
> > +{
> > +	struct xen_drm_front_drm_info *drm_info = fb->dev->dev_private;
> > +
> > +	drm_info->front_ops->fb_detach(drm_info->front_info,
> > +			xen_drm_front_fb_to_cookie(fb));
> > +	drm_gem_fb_destroy(fb);
> > +}
> > +
> > +static struct drm_framebuffer_funcs fb_funcs = {
> > +	.destroy = fb_destroy,
> > +};
> > +
> > +static struct drm_framebuffer *fb_create(struct drm_device *dev,
> > +		struct drm_file *filp, const struct drm_mode_fb_cmd2 *mode_cmd)
> > +{
> > +	struct xen_drm_front_drm_info *drm_info = dev->dev_private;
> > +	static struct drm_framebuffer *fb;
> > +	struct drm_gem_object *gem_obj;
> > +	int ret;
> > +
> > +	fb = drm_gem_fb_create_with_funcs(dev, filp, mode_cmd, &fb_funcs);
> > +	if (IS_ERR_OR_NULL(fb))
> > +		return fb;
> > +
> > +	gem_obj = drm_gem_object_lookup(filp, mode_cmd->handles[0]);
> > +	if (!gem_obj) {
> > +		DRM_ERROR("Failed to lookup GEM object\n");
> > +		ret = -ENOENT;
> > +		goto fail;
> > +	}
> > +
> > +	drm_gem_object_unreference_unlocked(gem_obj);
> > +
> > +	ret = drm_info->front_ops->fb_attach(
> > +			drm_info->front_info,
> > +			xen_drm_front_dbuf_to_cookie(gem_obj),
> > +			xen_drm_front_fb_to_cookie(fb),
> > +			fb->width, fb->height, fb->format->format);
> > +	if (ret < 0) {
> > +		DRM_ERROR("Back failed to attach FB %p: %d\n", fb, ret);
> > +		goto fail;
> > +	}
> > +
> > +	return fb;
> > +
> > +fail:
> > +	drm_gem_fb_destroy(fb);
> > +	return ERR_PTR(ret);
> > +}
> > +
> > +static const struct drm_mode_config_funcs mode_config_funcs = {
> > +	.fb_create = fb_create,
> > +	.atomic_check = drm_atomic_helper_check,
> > +	.atomic_commit = drm_atomic_helper_commit,
> > +};
> > +
> > +static int display_set_config(struct drm_simple_display_pipe *pipe,
> > +	struct drm_framebuffer *fb)
> > +{
> > +	struct xen_drm_front_drm_pipeline *pipeline =
> > +			to_xen_drm_pipeline(pipe);
> > +	struct drm_crtc *crtc = &pipe->crtc;
> > +	struct xen_drm_front_drm_info *drm_info = pipeline->drm_info;
> > +	int ret;
> > +
> > +	if (fb)
> > +		ret = drm_info->front_ops->mode_set(pipeline,
> > +				crtc->x, crtc->y,
> > +				fb->width, fb->height, fb->format->cpp[0] * 8,
> > +				xen_drm_front_fb_to_cookie(fb));
> > +	else
> > +		ret = drm_info->front_ops->mode_set(pipeline,
> > +				0, 0, 0, 0, 0,
> > +				xen_drm_front_fb_to_cookie(NULL));
> This is a bit much layering, the if (fb) case corresponds to the
> display_enable/disable hooks, pls fold that in instead of the indirection.
> simple helpers guarantee that when the display is on, then you have an fb.
1. Ok, the only reason for having this function was to keep
front_ops->mode_set calls at one place (will be refactored
to be a direct call, not via front_ops).
2. The if (fb) check was meant not to check if simple helpers
may give us some wrong value when we do not expect: there is
nothing wrong with them. The check was for 2 cases when this
function was called: with fb != NULL on display enable and
with fb == NULL on display disable, e.g. fb was used as a
flag in this check.
3. I will remove this function at all and will make direct calls
to the backend on .display_{enable|disable}
> Maybe we need to fix the docs, pls check and if that's not clear, submit a
> kernel-doc patch for the simple pipe helpers.
no, nothing wrong here, just see my reasoning above
> > +
> > +	if (ret)
> > +		DRM_ERROR("Failed to set mode to back: %d\n", ret);
> > +
> > +	return ret;
> > +}
> > +
> > +static void display_enable(struct drm_simple_display_pipe *pipe,
> > +		struct drm_crtc_state *crtc_state)
> > +{
> > +	struct drm_crtc *crtc = &pipe->crtc;
> > +	struct drm_framebuffer *fb = pipe->plane.state->fb;
> > +
> > +	if (display_set_config(pipe, fb) == 0)
> > +		drm_crtc_vblank_on(crtc);
> I get the impression your driver doesn't support vblanks (the page flip
> code at least looks like it's only generating a single event),
yes, this is true
>   you also
> don't have a enable/disable_vblank implementation.
this is because with my previous patches [1] these are now handled
by simple helpers, so no need to provide dummy ones in the driver
>   If there's no vblank
> handling then this shouldn't be needed.
yes, I will rework the code, please see below
> > +	else
> > +		DRM_ERROR("Failed to enable display\n");
> > +}
> > +
> > +static void display_disable(struct drm_simple_display_pipe *pipe)
> > +{
> > +	struct drm_crtc *crtc = &pipe->crtc;
> > +
> > +	display_set_config(pipe, NULL);
> > +	drm_crtc_vblank_off(crtc);
> > +	/* final check for stalled events */
> > +	if (crtc->state->event && !crtc->state->active) {
> > +		unsigned long flags;
> > +
> > +		spin_lock_irqsave(&crtc->dev->event_lock, flags);
> > +		drm_crtc_send_vblank_event(crtc, crtc->state->event);
> > +		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> > +		crtc->state->event = NULL;
> > +	}
> > +}
> > +
> > +void xen_drm_front_kms_on_frame_done(
> > +		struct xen_drm_front_drm_pipeline *pipeline,
> > +		uint64_t fb_cookie)
> > +{
> > +	drm_crtc_handle_vblank(&pipeline->pipe.crtc);
> Hm, again this doesn't look like real vblank, but only a page-flip done
> event. If that's correct then please don't use the vblank machinery, but
> just store the event internally (protected with your own private spinlock)
Why can't I use &dev->event_lock? Anyways for handling
page-flip events I will need to lock on it, so I can do
drm_crtc_send_vblank_event?
> and send it out using drm_crtc_send_vblank_event directly. No calls to
> arm_vblank_event or any of the other vblank infrastructure should be
> needed.
will re-work, e.g. will store drm_pending_vblank_event
on .display_update and send out on page flip event from the
backend
> Also please remove the drm_vblank_init() call, since your hw doesn't
> really have vblanks. And exposing vblanks to userspace without
> implementing them is confusing.
will remove all vblank handling at all with the re-work above
> > +}
> > +
> > +static void display_send_page_flip(struct drm_simple_display_pipe *pipe,
> > +		struct drm_plane_state *old_plane_state)
> > +{
> > +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(
> > +			old_plane_state->state, &pipe->plane);
> > +
> > +	/*
> > +	 * If old_plane_state->fb is NULL and plane_state->fb is not,
> > +	 * then this is an atomic commit which will enable display.
> > +	 * If old_plane_state->fb is not NULL and plane_state->fb is,
> > +	 * then this is an atomic commit which will disable display.
> > +	 * Ignore these and do not send page flip as this framebuffer will be
> > +	 * sent to the backend as a part of display_set_config call.
> > +	 */
> > +	if (old_plane_state->fb && plane_state->fb) {
> > +		struct xen_drm_front_drm_pipeline *pipeline =
> > +				to_xen_drm_pipeline(pipe);
> > +		struct xen_drm_front_drm_info *drm_info = pipeline->drm_info;
> > +		int ret;
> > +
> > +		ret = drm_info->front_ops->page_flip(drm_info->front_info,
> > +				pipeline->index,
> > +				xen_drm_front_fb_to_cookie(plane_state->fb));
> > +		pipeline->pgflip_last_error = ret;
> > +		if (ret) {
> > +			DRM_ERROR("Failed to send page flip request to backend: %d\n", ret);
> > +			/*
> > +			 * As we are at commit stage the DRM core will anyways
> > +			 * wait for the vblank and knows nothing about our
> > +			 * failure. The best we can do is to handle
> > +			 * vblank now, so there is no vblank/flip_done
> > +			 * time outs
> > +			 */
> > +			drm_crtc_handle_vblank(&pipeline->pipe.crtc);
> > +		}
> > +	}
> > +}
> > +
> > +static int display_prepare_fb(struct drm_simple_display_pipe *pipe,
> > +		struct drm_plane_state *plane_state)
> > +{
> > +	struct xen_drm_front_drm_pipeline *pipeline =
> > +			to_xen_drm_pipeline(pipe);
> > +
> > +	if (pipeline->pgflip_last_error) {
> > +		int ret;
> > +
> > +		/* if previous page flip didn't succeed then report the error */
> > +		ret = pipeline->pgflip_last_error;
> > +		/* and let us try to page flip next time */
> > +		pipeline->pgflip_last_error = 0;
> > +		return ret;
> > +	}
> Nope, this isn't how the uapi works. If your flips fail then we might need
> to add some error status thing to the drm events, but you can't make the
> next flip fail.
Well, yes, there is no way for me to tell that the page flip
has failed, so this is why I tried to do this workaround with
the next page-flip. The reason for that is that if, for example,
we are disconnected from the backend for some reason, there is
no way for me to tell the user-space that hey, please, do not
send any other page flips. If backend can recover and that was
a one time error then yes, the code I have will do wrong thing
(fail the current page flip), but if the error state is persistent
then I will be able to tell the user-space to stop by returning errors.
This is kind of trade-off which I am not sure how to solve correctly.

Do you think I can remove this workaround completely?
> -Daniel
>
> > +	return drm_gem_fb_prepare_fb(&pipe->plane, plane_state);
> > +}
> > +
> > +static void display_update(struct drm_simple_display_pipe *pipe,
> > +		struct drm_plane_state *old_plane_state)
> > +{
> > +	struct drm_crtc *crtc = &pipe->crtc;
> > +	struct drm_pending_vblank_event *event;
> > +
> > +	event = crtc->state->event;
> > +	if (event) {
> > +		struct drm_device *dev = crtc->dev;
> > +		unsigned long flags;
> > +
> > +		crtc->state->event = NULL;
> > +
> > +		spin_lock_irqsave(&dev->event_lock, flags);
> > +		if (drm_crtc_vblank_get(crtc) == 0)
> > +			drm_crtc_arm_vblank_event(crtc, event);
> > +		else
> > +			drm_crtc_send_vblank_event(crtc, event);
> > +		spin_unlock_irqrestore(&dev->event_lock, flags);
> > +	}
> > +	/*
> > +	 * Send page flip request to the backend *after* we have event armed/
> > +	 * sent above, so on page flip done event from the backend we can
> > +	 * deliver it while handling vblank.
> > +	 */
> > +	display_send_page_flip(pipe, old_plane_state);
> > +}
> > +
> > +static const struct drm_simple_display_pipe_funcs display_funcs = {
> > +	.enable = display_enable,
> > +	.disable = display_disable,
> > +	.prepare_fb = display_prepare_fb,
> > +	.update = display_update,
> > +};
> > +
> > +static int display_pipe_init(struct xen_drm_front_drm_info *drm_info,
> > +		int index, struct xen_drm_front_cfg_connector *cfg,
> > +		struct xen_drm_front_drm_pipeline *pipeline)
> > +{
> > +	struct drm_device *dev = drm_info->drm_dev;
> > +	const uint32_t *formats;
> > +	int format_count;
> > +	int ret;
> > +
> > +	pipeline->drm_info = drm_info;
> > +	pipeline->index = index;
> > +	pipeline->height = cfg->height;
> > +	pipeline->width = cfg->width;
> > +
> > +	ret = xen_drm_front_conn_init(drm_info, &pipeline->conn);
> > +	if (ret)
> > +		return ret;
> > +
> > +	formats = xen_drm_front_conn_get_formats(&format_count);
> > +
> > +	return drm_simple_display_pipe_init(dev, &pipeline->pipe,
> > +			&display_funcs, formats, format_count,
> > +			NULL, &pipeline->conn);
> > +}
> > +
> > +int xen_drm_front_kms_init(struct xen_drm_front_drm_info *drm_info)
> > +{
> > +	struct drm_device *dev = drm_info->drm_dev;
> > +	int i, ret;
> > +
> > +	drm_mode_config_init(dev);
> > +
> > +	dev->mode_config.min_width = 0;
> > +	dev->mode_config.min_height = 0;
> > +	dev->mode_config.max_width = 4095;
> > +	dev->mode_config.max_height = 2047;
> > +	dev->mode_config.funcs = &mode_config_funcs;
> > +
> > +	for (i = 0; i < drm_info->cfg->num_connectors; i++) {
> > +		struct xen_drm_front_cfg_connector *cfg =
> > +				&drm_info->cfg->connectors[i];
> > +		struct xen_drm_front_drm_pipeline *pipeline =
> > +				&drm_info->pipeline[i];
> > +
> > +		ret = display_pipe_init(drm_info, i, cfg, pipeline);
> > +		if (ret) {
> > +			drm_mode_config_cleanup(dev);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	drm_mode_config_reset(dev);
> > +	return 0;
> > +}
> > diff --git a/drivers/gpu/drm/xen/xen_drm_front_kms.h b/drivers/gpu/drm/xen/xen_drm_front_kms.h
> > new file mode 100644
> > index 000000000000..65a50033bb9b
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xen/xen_drm_front_kms.h
> > @@ -0,0 +1,30 @@
> > +/*
> > + *  Xen para-virtual DRM device
> > + *
> > + *   This program is free software; you can redistribute it and/or modify
> > + *   it under the terms of the GNU General Public License as published by
> > + *   the Free Software Foundation; either version 2 of the License, or
> > + *   (at your option) any later version.
> > + *
> > + *   This program is distributed in the hope that it will be useful,
> > + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + *   GNU General Public License for more details.
> > + *
> > + * Copyright (C) 2016-2018 EPAM Systems Inc.
> > + *
> > + * Author: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
> > + */
> > +
> > +#ifndef __XEN_DRM_FRONT_KMS_H_
> > +#define __XEN_DRM_FRONT_KMS_H_
> > +
> > +#include "xen_drm_front_drv.h"
> > +
> > +int xen_drm_front_kms_init(struct xen_drm_front_drm_info *drm_info);
> > +
> > +void xen_drm_front_kms_on_frame_done(
> > +		struct xen_drm_front_drm_pipeline *pipeline,
> > +		uint64_t fb_cookie);
> > +
> > +#endif /* __XEN_DRM_FRONT_KMS_H_ */
> > --
> > 2.7.4
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
[1] https://patchwork.kernel.org/patch/10211997/
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel