Re: [PATCH V2] drm/vkms: Add vblank events simulated by hrtimers

On Thu, Jul 05, 2018 at 12:48:43AM -0300, Rodrigo Siqueira wrote:

This commit adds regular vblank events simulated through hrtimers, which is a feature required by VKMS to mimic real hardware. Notice that keeping the periodicity as close as possible to the one specified in user space represents a vital constraint. In this sense, the callback function implements an algorithm based on module operations that avoids the accumulation of errors.

Assuming we begin operating at timestamp 0, every event should happen at timestamps that are multiples of the requested period, i.e., current_timestamp % period == 0. Since we do *not* generally start at timestamp 0, we calculate the modulus of this initial timestamp and try to make all following events happen when current_timestamp % period == initial_timestamp % period (variables “current_offset” and “base_offset”). We do this by measuring the difference between them at each iteration and adjusting the next waiting period accordingly.

Signed-off-by: Rodrigo Siqueira rodrigosiqueiramelo@gmail.com

Looks good. I've done a detailed review and found a bunch of things to polish.

When resending patches please include a short summary of the changes you've done. This helps reviewers a lot. E.g. for this one here:

Changes since v1: - Compute the timer interval using the mode.

For the next revision you keep that and add

Changes since v2: - Use the timing information already computed by core code. - Use absolute timer mode - and-so-on ...

Cheers, Daniel

---

drivers/gpu/drm/vkms/vkms_crtc.c | 99 +++++++++++++++++++++++++++++++- drivers/gpu/drm/vkms/vkms_drv.c | 9 +++ drivers/gpu/drm/vkms/vkms_drv.h | 20 +++++++ 3 files changed, 126 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c index 84cc05506b09..35d39d25df34 100644 --- a/drivers/gpu/drm/vkms/vkms_crtc.c +++ b/drivers/gpu/drm/vkms/vkms_crtc.c @@ -10,6 +10,83 @@ #include <drm/drm_atomic_helper.h> #include <drm/drm_crtc_helper.h>

+static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) +{

• struct vkms_output *output = container_of(timer, struct vkms_output,

• 				  vblank_hrtimer);
  

• struct drm_crtc *crtc = &output->crtc;
• ktime_t new_period, current_timestamp, diff_times, current_offset,

• candidate_expires;
  

• unsigned long flags;
• int ret_overrun;
• bool ret;
• current_timestamp = ktime_get();
• current_offset = current_timestamp % output->period_ns;
• diff_times = ktime_sub(current_offset, output->base_offset);
• candidate_expires = ktime_sub(current_timestamp, diff_times);
• if (candidate_expires > output->expires)

• output->expires = candidate_expires;
  

• ret = drm_crtc_handle_vblank(crtc);
• if (!ret)

• DRM_ERROR("vkms failure on handling vblank");
  

• if (output->event) {

You're accessing output->event without holding locks, that's not ok. But see below, you can remove all that code.

• spin_lock_irqsave(&crtc->dev->event_lock, flags);
  

• drm_crtc_send_vblank_event(crtc, output->event);
  

• output->event = NULL;
  

• spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
  

• drm_crtc_vblank_put(crtc);
  

• }
• current_timestamp = ktime_get();
• current_offset = current_timestamp % output->period_ns;
• diff_times = ktime_sub(output->base_offset, current_offset);
• new_period = ktime_add(output->period_ns, diff_times);
• output->expires = ktime_add(current_timestamp, new_period);
• ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer, new_period);
• return HRTIMER_RESTART;

+}

+static int vkms_enable_vblank(struct drm_crtc *crtc) +{

• struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
• unsigned long period = 1000 * crtc->mode.htotal * crtc->mode.vtotal /

• 	       crtc->mode.clock;
  

General note: Diving stuff in the kernel isn't easy, you need to use division macros otherwise your code won't build on 32bit.

Wrt the calculation itself: That's already done for you in drm_calc_timestamping_constants(), no need to reinvent that wheel.

• ktime_t current_timestamp;
• hrtimer_init(&out->vblank_hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);

Can't we use absolute timer mode here? That avoids all the timestampt computations.

• out->vblank_hrtimer.function = &vkms_vblank_simulate;
• out->period_ns = ktime_set(0, period * US_TO_NS);
• current_timestamp = ktime_get();
• out->base_offset = current_timestamp % out->period_ns;
• out->expires = ktime_add(current_timestamp, out->period_ns);
• hrtimer_start(&out->vblank_hrtimer, out->period_ns, HRTIMER_MODE_REL);
• return 0;

+}

+static void vkms_disable_vblank(struct drm_crtc *crtc) +{

• struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
• hrtimer_cancel(&out->vblank_hrtimer);

+}

+bool vkms_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,

• 	       int *max_error, ktime_t *vblank_time,
  

• 	       bool in_vblank_irq)
  

+{

• struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
• struct vkms_output *output = &vkmsdev->output;
• *vblank_time = output->expires;
• return true;

+}

static const struct drm_crtc_funcs vkms_crtc_funcs = { .set_config = drm_atomic_helper_set_config, .destroy = drm_crtc_cleanup, @@ -17,6 +94,8 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = { .reset = drm_atomic_helper_crtc_reset, .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,

• .enable_vblank = vkms_enable_vblank,
• .disable_vblank = vkms_disable_vblank,

};

static int vkms_crtc_atomic_check(struct drm_crtc *crtc, @@ -28,11 +107,27 @@ static int vkms_crtc_atomic_check(struct drm_crtc *crtc, static void vkms_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_crtc_state *old_state) {

• drm_crtc_vblank_on(crtc);

drm_crtc_vblank_off is missing. That should result in some complaints when you disable a crtc again ... Have you not seen any backtraces in dmesg?

+}

+static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,

• 		   struct drm_crtc_state *old_s)
  

+{

• struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
• if (crtc->state->event) {

• crtc->state->event->pipe = drm_crtc_index(crtc);
  

• WARN_ON(drm_crtc_vblank_get(crtc) != 0);
  

• vkms_output->event = crtc->state->event;
  

You're missing locking here for output->event. Also I expect that you're hitting the WARN_ON above (or you would, if you would call drm_crtc_vblank_off).

Much simpler implementation is if you use drm_crtc_arm_vblank_event, plus the fallback to sending the event out directly if the crtc is disabled. See tegra_crtc_atomic_begin() for an example.

Also for the long-term plans we have with vkms with the crc stuff it's better to put this all into the atomic_flush callback, not the atomic_begin callback.

• crtc->state->event = NULL;
  

• }

}

static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {

• .atomic_check = vkms_crtc_atomic_check,
• .atomic_enable = vkms_crtc_atomic_enable,

• .atomic_check = vkms_crtc_atomic_check,
• .atomic_enable = vkms_crtc_atomic_enable,
• .atomic_begin = vkms_crtc_atomic_begin,

};

int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c index fe93f8c17997..6368048c6e54 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.c +++ b/drivers/gpu/drm/vkms/vkms_drv.c @@ -55,6 +55,7 @@ static struct drm_driver vkms_driver = { .dumb_create = vkms_dumb_create, .dumb_map_offset = vkms_dumb_map, .gem_vm_ops = &vkms_gem_vm_ops,

• .get_vblank_timestamp = vkms_get_vblank_timestamp,

Ah, the last legacy hook where we don't yet have an option to use drm_crtc_funcs instead. For now this is good here, but I think it'd be great to do a follow-up task to clean this up, like we've already done with the enable/disable_vblank hooks.

.name = DRIVER_NAME, .desc = DRIVER_DESC, @@ -102,6 +103,14 @@ static int __init vkms_init(void) goto out_fini; }

• vkms_device->drm.irq_enabled = true;
• ret = drm_vblank_init(&vkms_device->drm, 1);
• if (ret) {

• DRM_ERROR("Failed to vblank\n");
  

• goto out_fini;
  

• }
• ret = vkms_modeset_init(vkms_device); if (ret) goto out_unregister;

diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index 76f1720f81a5..b69a223bdfe6 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 <linux/hrtimer.h>

#define XRES_MIN 32 #define YRES_MIN 32 @@ -15,6 +16,9 @@ #define XRES_MAX 8192 #define YRES_MAX 8192

+#define DEFAULT_VBLANK_NS 16666666 +#define US_TO_NS 1000

static const u32 vkms_formats[] = { DRM_FORMAT_XRGB8888, }; @@ -23,6 +27,11 @@ struct vkms_output { struct drm_crtc crtc; struct drm_encoder encoder; struct drm_connector connector;

• struct hrtimer vblank_hrtimer;
• ktime_t period_ns;
• ktime_t base_offset;
• ktime_t expires;
• struct drm_pending_vblank_event *event;

};

struct vkms_device { @@ -37,9 +46,20 @@ struct vkms_gem_object { struct page **pages; };

+#define drm_crtc_to_vkms_output(target) \

• container_of(target, struct vkms_output, crtc)

+#define drm_device_to_vkms_device(target) \

• container_of(target, struct vkms_device, drm)

+/* CRTC */ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, struct drm_plane *primary, struct drm_plane *cursor);

+bool vkms_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,

• 	       int *max_error, ktime_t *vblank_time,
  

• 	       bool in_vblank_irq);
  

int vkms_output_init(struct vkms_device *vkmsdev);

struct drm_plane *vkms_plane_init(struct vkms_device *vkmsdev);

2.18.0