lkml.org 
[lkml]   [2018]   [Mar]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 7/9] drm/xen-front: Implement KMS/connector handling
From
Date
On 03/06/2018 09:22 AM, Daniel Vetter wrote:
> 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.
ok, does make sense, will rework without yet another function
>> 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.
I just was clarifying on the need for my own private lock ;)
>>> 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.
Ah, then I'll need to plumb in connector hotplug machinery.
Will add that so I can report errors
> 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.
Good point, thank you
> -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

\
 
 \ /
  Last update: 2018-03-06 08:30    [W:0.139 / U:6.056 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site