On Tue, Jul 17, 2018 at 02:52:20AM +0300, Haneen Mohammed wrote:
On Mon, Jul 16, 2018 at 02:22:56PM -0400, Sean Paul wrote:
On Sat, Jul 14, 2018 at 03:23:32PM +0300, Haneen Mohammed wrote:
Implement the set_crc_source() callback. Compute CRC using crc32 on the visible part of the framebuffer. Use ordered workqueue to compute and add CRC at the end of a vblank.
Use appropriate synchronization methods since the crc computation must be atomic wrt the generated vblank event for a given atomic update,
Signed-off-by: Haneen Mohammed hamohammed.sa@gmail.com
Hey Haneen, Thanks for revising this patch set. Things are looking good across the series, just a few more comments :-)
Thank you so much for the review!
Changes in v2:
- Include this patch in this patchset.
drivers/gpu/drm/vkms/Makefile | 2 +- drivers/gpu/drm/vkms/vkms_crtc.c | 60 ++++++++++++++++++++++++++----- drivers/gpu/drm/vkms/vkms_drv.c | 1 + drivers/gpu/drm/vkms/vkms_drv.h | 21 +++++++++++ drivers/gpu/drm/vkms/vkms_plane.c | 10 ++++++ 5 files changed, 85 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile index 986297da51bf..37966914f70b 100644 --- a/drivers/gpu/drm/vkms/Makefile +++ b/drivers/gpu/drm/vkms/Makefile @@ -1,3 +1,3 @@ -vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o +vkms-y := vkms_drv.o vkms_plane.o vkms_output.o vkms_crtc.o vkms_gem.o vkms_crc.o
obj-$(CONFIG_DRM_VKMS) += vkms.o diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c index 26babb85ca77..f3a674db33b8 100644 --- a/drivers/gpu/drm/vkms/vkms_crtc.c +++ b/drivers/gpu/drm/vkms/vkms_crtc.c @@ -10,18 +10,36 @@ #include <drm/drm_atomic_helper.h> #include <drm/drm_crtc_helper.h>
-static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) +static void _vblank_handle(struct vkms_output *output) {
- struct vkms_output *output = container_of(timer, struct vkms_output,
vblank_hrtimer);- int ret_overrun;
struct vkms_crtc_state *state = to_vkms_crtc_state(crtc->state); bool ret;
int crc_enabled = 0;
spin_lock(&output->lock);
crc_enabled = output->crc_enabled;
Aside from the implicit bool -> int cast, I don't think you need this local var, just use output->crc_enabled directly below.
ret = drm_crtc_handle_vblank(crtc); if (!ret) DRM_ERROR("vkms failure on handling vblank");
This would be more useful with the error code printed.
I think this only returns false on failure. Also I've noticed most of the usage of drm_crtc_handle_vblank don't check the return value, should I do the same as well and drop ret and error message?
Ahh, I didn't see that ret was bool. In that case, the error message is fine. It's always good practice to assume that if a function returns a status, it's worth checking.
- if (state && crc_enabled) {
state->n_frame = drm_crtc_accurate_vblank_count(crtc);queue_work(output->crc_workq, &state->crc_work);- }
- spin_unlock(&output->lock);
+}
+static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) +{
- struct vkms_output *output = container_of(timer, struct vkms_output,
vblank_hrtimer);- int ret_overrun;
- _vblank_handle(output);
- ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer, output->period_ns);
@@ -97,6 +115,8 @@ vkms_atomic_crtc_duplicate_state(struct drm_crtc *crtc)
__drm_atomic_helper_crtc_duplicate_state(crtc, &vkms_state->base);
- INIT_WORK(&vkms_state->crc_work, vkms_crc_work_handle);
- return &vkms_state->base;
}
@@ -104,11 +124,18 @@ static void vkms_atomic_crtc_destroy_state(struct drm_crtc *crtc, struct drm_crtc_state *state) { struct vkms_crtc_state *vkms_state;
struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
vkms_state = to_vkms_crtc_state(state);
__drm_atomic_helper_crtc_destroy_state(state);
- kfree(vkms_state);
- if (vkms_state) {
flush_workqueue(vkms_out->crc_workq);I'm a little worried about this bit. Since the workqueue is per-output, is it possible you'll be waiting for more frames to complete than you need to be?
I see, maybe I should flush per work_struct instead?
Yeah, that would make more sense IMO.
drm_framebuffer_put(&vkms_state->data.fb);memset(&vkms_state->data, 0, sizeof(struct vkms_crc_data));kfree(vkms_state);- }
}
static const struct drm_crtc_funcs vkms_crtc_funcs = { @@ -120,6 +147,7 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = { .atomic_destroy_state = vkms_atomic_crtc_destroy_state, .enable_vblank = vkms_enable_vblank, .disable_vblank = vkms_disable_vblank,
- .set_crc_source = vkms_set_crc_source,
};
static void vkms_crtc_atomic_enable(struct drm_crtc *crtc, @@ -134,26 +162,37 @@ static void vkms_crtc_atomic_disable(struct drm_crtc *crtc, drm_crtc_vblank_off(crtc); }
+static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,
struct drm_crtc_state *old_crtc_state)+{
- struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
- spin_lock_irq(&vkms_output->lock);
Hmm, you can't lock across atomic_begin/flush. What is this protecting against?
I did this per Daniel recommendation: 1- spin_lock_irq -> vkms_crtc_atomic_begin 2- update vkms_crc_data and get ref to fb -> vkms_primary_plane_update 3- spin_unlock_irq after vblank event -> vkms_crtc_atomic_flush
I think I'm interpreting his mail a little differently. The point of the spinlock is to protect the pointer to the crc_data in vkms_crtc (right now it's not a pointer, but it should be).
So I think you need to do the following: - Change crc_data to a pointer - In plane_update, allocate new memory to hold crc_data and, under the spinlock, update the crc_data to point to the new memory (add a WARN_ON if the pointer is NULL) - In the hrtimer, while holding the spinlock, take the pointer from crc_data into a local variable and clear the crtc->crc_data pointer - Pass the pointer (from the local variable) to the crc_worker
I don't think you need to hold the spinlock across the atomic hooks, just grab it in plane_update.
Sean
+}
static void vkms_crtc_atomic_flush(struct drm_crtc *crtc, struct drm_crtc_state *old_crtc_state) {
- unsigned long flags;
struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
if (crtc->state->event) {
spin_lock_irqsave(&crtc->dev->event_lock, flags);
spin_lock(&crtc->dev->event_lock);What's the rationale behind this change?
hm I am not sure if this is correct, but I assume because spin_lock_irq in atomic_begin would disable interrupts and since this is before we unlock vkms_output->lock with spin_unlock_irq so irqsave here is not necessary?
Once you limit the scope of the spinlock above, you will want to revert this change.
if (drm_crtc_vblank_get(crtc) != 0) drm_crtc_send_vblank_event(crtc, crtc->state->event); else drm_crtc_arm_vblank_event(crtc, crtc->state->event);
spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
spin_unlock(&crtc->dev->event_lock);crtc->state->event = NULL; }
spin_unlock_irq(&vkms_output->lock);
}
static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
- .atomic_begin = vkms_crtc_atomic_begin, .atomic_flush = vkms_crtc_atomic_flush, .atomic_enable = vkms_crtc_atomic_enable, .atomic_disable = vkms_crtc_atomic_disable,
@@ -162,6 +201,7 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = { int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, struct drm_plane *primary, struct drm_plane *cursor) {
struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc); int ret;
ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor,
@@ -173,5 +213,9 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
- spin_lock_init(&vkms_out->lock);
- vkms_out->crc_workq = alloc_ordered_workqueue("vkms_crc_workq", 0);
- return ret;
} diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c index 37aa2ef33b21..5d78bd97e69c 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.c +++ b/drivers/gpu/drm/vkms/vkms_drv.c @@ -46,6 +46,7 @@ static void vkms_release(struct drm_device *dev) platform_device_unregister(vkms->platform); drm_mode_config_cleanup(&vkms->drm); drm_dev_fini(&vkms->drm);
- destroy_workqueue(vkms->output.crc_workq);
}
static struct drm_driver vkms_driver = { diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index bf811d0ec349..95985649768c 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.h +++ b/drivers/gpu/drm/vkms/vkms_drv.h @@ -20,12 +20,23 @@ static const u32 vkms_formats[] = { DRM_FORMAT_XRGB8888, };
+struct vkms_crc_data {
- struct drm_rect src;
- struct drm_framebuffer fb;
+};
/**
- vkms_crtc_state - Driver specific CRTC state
- @base: base CRTC state
- @crc_work: work struct to compute and add CRC entries
- @data: data required for CRC computation
*/
- @n_frame: frame number for computed CRC
struct vkms_crtc_state { struct drm_crtc_state base;
- struct work_struct crc_work;
- struct vkms_crc_data data;
- unsigned int n_frame;
};
struct vkms_output { @@ -35,6 +46,11 @@ struct vkms_output { struct hrtimer vblank_hrtimer; ktime_t period_ns; struct drm_pending_vblank_event *event;
- bool crc_enabled;
Where is this set to true?
Oh sorry I forgot to add a vkms_crc.c which sets crc_enabled in set_crc_source callback!
I'll work on fixing the issues you mentioned here and the other patches as well, thank you so much again!
Thanks for working through this!
Sean
Haneen
- /* ordered wq for crc_work */
- struct workqueue_struct *crc_workq;
- /* protects concurrent access to crc_data */
- spinlock_t lock;
};
struct vkms_device { @@ -95,4 +111,9 @@ void *vkms_gem_vmap(struct drm_gem_object *obj);
void vkms_gem_vunmap(struct drm_gem_object *obj);
+/* CRC Support */ +int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
size_t *values_cnt);+void vkms_crc_work_handle(struct work_struct *work);
#endif /* _VKMS_DRV_H_ */ diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c index aaf7c35faed6..1e0ee8ca8bd6 100644 --- a/drivers/gpu/drm/vkms/vkms_plane.c +++ b/drivers/gpu/drm/vkms/vkms_plane.c @@ -24,6 +24,16 @@ static const struct drm_plane_funcs vkms_plane_funcs = { static void vkms_primary_plane_update(struct drm_plane *plane, struct drm_plane_state *old_state) {
- struct vkms_crtc_state *state;
- if (!plane->state->crtc || !plane->state->fb)
return;- state = to_vkms_crtc_state(plane->state->crtc->state);
- memcpy(&state->data.src, &plane->state->src, sizeof(struct drm_rect));
- memcpy(&state->data.fb, plane->state->fb,
sizeof(struct drm_framebuffer));- drm_framebuffer_get(&state->data.fb);
}
static int vkms_plane_atomic_check(struct drm_plane *plane,
2.17.1
-- Sean Paul, Software Engineer, Google / Chromium OS