Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 6 Mar 2018 08:22:05 +0100
From:   Daniel Vetter <daniel@ffwll.ch>
To:     Oleksandr Andrushchenko <andr2000@gmail.com>
Cc:     xen-devel@lists.xenproject.org, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org, airlied@linux.ie,
        daniel.vetter@intel.com, seanpaul@chromium.org,
        gustavo@padovan.org, jgross@suse.com, boris.ostrovsky@oracle.com,
        konrad.wilk@oracle.com,
        Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Subject: Re: [PATCH 7/9] drm/xen-front: Implement KMS/connector handling
Message-ID: <20180306072205.GP22212@phenom.ffwll.local>
Mail-Followup-To: Oleksandr Andrushchenko <andr2000@gmail.com>,
        xen-devel@lists.xenproject.org, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org, airlied@linux.ie,
        daniel.vetter@intel.com, seanpaul@chromium.org, gustavo@padovan.org,
        jgross@suse.com, boris.ostrovsky@oracle.com, konrad.wilk@oracle.com,
        Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
References: <1519200222-20623-1-git-send-email-andr2000@gmail.com>
 <1519200222-20623-8-git-send-email-andr2000@gmail.com>
 <20180305092353.GI22212@phenom.ffwll.local>
 <8132411b-ee50-fe42-2e62-8816a1f85433@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <8132411b-ee50-fe42-2e62-8816a1f85433@gmail.com>
User-Agent: Mutt/1.9.3 (2018-01-21)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Mar 05, 2018 at 02:59:23PM +0200, Oleksandr Andrushchenko wrote:
> 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@epam.com>
> > > 
> > > 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@epam.com>
> > 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@epam.com>
> > > + */
> > > +
> > > +#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@epam.com>
> > > + */
> > > +
> > > +#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@epam.com>
> > > + */
> > > +
> > > +#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.

Yeah that's what I meant - it is needlessly confusing: You get 2 explicit
enable/disable callbacks, then you squash them into 1 function call, only
to require an

if (do_I_need_to_enable_or_disable) {
	/* code that really should be directly put in the enable callback */
} else {
	/* code that really should be directly put in the enable callback */
}

Just a bit of indirection where I didnt' see the point.

Aside for why this matters: When refactoring the entire subsystem you need
to be able to quickly understand how all the drivers work in a specific
case, without being an expert on that driver. If there's very little
indirection between the shared drm concepts/structs/callbacks and the
actual driver code, then that's easy. If there's a bunch of callback
layers or indirections like the above, you make subsystem refactoring
harder for no reason. And in upstream we optimize for the overall
subsystem, not individual drivers.

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

Yeah you can reuse the event_lock too, that's what many drivers do.

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

Yes. If you want to tell userspace that the backend is gone, send a
hotplug uevent and update the connector status to disconnected. Hotplug
uevents is how we tell userspace about asynchronous changes. We also have
special stuff to signal display cable issue that might require picking a
lower resolution (DP link training) and when HDCP encryption failed.

Sending back random errors on pageflips just confuses the compositor, and
all correctly working compositors will listen to hotplug events and
reprobe all the outputs and change the configuration if necessary.
-Daniel

> > -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@epam.com>
> > > + */
> > > +
> > > +#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@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> [1] https://patchwork.kernel.org/patch/10211997/
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch