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 --- 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) { + 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; + ktime_t current_timestamp; + + hrtimer_init(&out->vblank_hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); + 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); +} + +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; + 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,
.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);
On Thu, Jul 05, 2018 at 12:48:43AM -0300, Rodrigo Siqueira wrote:
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
You're accessing output->event without holding locks, that's not ok. But see below, you can remove all that code.
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.
Can't we use absolute timer mode here? That avoids all the timestampt computations.
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?
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.
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.
Hi and thanks for all the feedback, I will work on the suggestions you sent, but I have some doubts:
On 07/05, Chris Wilson wrote:
I did not understand the question. hrtimer_forward_now calculates the absolute timestamp from the relative value provided, this is what I am using. In any case, it would be easy to switch to absolute mode, but the code would not be smaller (or larger).
Indeed, output->expires can be removed; as for the rest of the code, that depends on the answer to question 2 below.
I have two questions:
1. The timestamp that is returned to userspace is (A) the timestamp when the interrupt was actually handled, allowing applications to detect when there is some irregularities in the interrupt handling timing, or (B) the timestamp when the current interrupt was *scheduled* to happen, allowing applications to detect overruns but not variations in the interrupt handling timing?
2. If I use hrtimer with a period of 1s and return HRTIMER_RESTART, will I be called back (A) 1s after the previous iteration was *scheduled to start* (i.e., I will actually be called back at regular intervals, so that after 1,000 iterations approximately 1,000s have elapsed) or (B) 1s after the previous iteration *ended* (i.e., I will be called back at intervals of 1s + the average processing time of the function, so that after 1,000 iterations significantly more than 1,000s have elapsed)?
The code I wrote assumes the answer to both questions is (B). If the answer to the second question is A, the code can indeed be made much simpler; if the answer to the first question is A, I have not been able to keep timing within the expected strict limits of the IGT test in a VM (maybe on physical hardware things would go better).
Thanks
Quoting Rodrigo Siqueira (2018-07-05 17:28:20)
It was a question to Daniel, trying to illustrate that REL is just a convenience in the htrimer api over ABS.
When the previous hrtimer actually occurred rolled back to the actual start of frame, i.e.
hrtimer_forward_now(hrtimer, vblank_interval); output->last_vblank_timestamp = ktime_sub(hrtimer->expires, vblank_interval);
userspace is mainly oblivious to the delivery latency (although it can measure it as clock_gettim() - vblank->timestamp), but is concerned about knowing what the current and next vblank HW timing will be.
No. When you get callback is controlled by the value you set in hrtimer->expires before you return RESTART. All RESTART does is immediately requeue the hrtimer, but more efficiently than calling hrtimer_start yourself. It is the call to hrtimer_forward that computes the next absolute hrtimer->expires based on the current time and your vblank interval.
The code you wrote is actually A. hrtimer_forward() rolls the absolute timer forward such that its expiry is the next interval after now. It is while (hrtimer->expires < now) hrtimer->expires += interval; -Chris
Now I got it! Really thanks for the help. I will prepare a V3.
On Thu, Jul 5, 2018 at 1:45 PM, Chris Wilson chris@chris-wilson.co.uk wrote:
On Thu, Jul 5, 2018 at 6:45 PM, Chris Wilson chris@chris-wilson.co.uk wrote:
Yeah I got confused and Chris corrected hat. _REL seems fine, just don't duplicate the timestamp computation. -Daniel
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Daniel Vetter -
Rodrigo Siqueira