Re: [RFC 3/3] drm/vkms: Implement CRC debugfs API
On Thu, Jun 28, 2018 at 01:40:08PM +0200, Daniel Vetter wrote:
> On Thu, Jun 28, 2018 at 10:07 AM, Daniel Vetter wrote:
> > On Thu, Jun 28, 2018 at 12:24:35AM +0300, Haneen Mohammed wrote:
> >> Implement the .set_crc_source() callback.
> >> Compute CRC using crc32 on the visible part of the framebuffer.
> >> Use work_struct to compute and add CRC at the end of a vblank.
> >>
> >> Signed-off-by: Haneen Mohammed
> >
> > Ok locking review. I still don't have a clear idea how to best fix this,
> > but oh well.
> >
> >> ---
> >> drivers/gpu/drm/vkms/vkms_crtc.c | 76
> >> drivers/gpu/drm/vkms/vkms_drv.h | 16 +++
> >> 2 files changed, 92 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c
> >> b/drivers/gpu/drm/vkms/vkms_crtc.c
> >> index 73aae129c37d..d78934b76e33 100644
> >> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> >> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> >> @@ -7,8 +7,78 @@
> >> */
> >>
> >> #include "vkms_drv.h"
> >> +#include
> >> #include
> >> #include
> >> +#include
> >> +
> >> +uint32_t _vkms_get_crc(struct drm_framebuffer *fb)
> >> +{
> >> + struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
> >> + struct vkms_gem_object *vkms_obj = container_of(gem_obj,
> >> + struct
> >> vkms_gem_object,
> >> + gem);
> >> + u32 crc = 0;
> >> + int i = 0;
> >> + struct drm_plane_state *state = vkms_obj->plane_state;
> >
> > vkms_obj->plane_state is the first locking issue: You store the pointer
> > without any locks or synchronization, which means the crc worker here
> > could access is while it's being changed, resulting in an inconsistent
> > crc. Note that the compiler/cpu is allowed to read a pointer multiple
> > times if it's not protected by locking/barriers, so all kinds of funny
> > stuff can happen here.
> >
> > Second issue is that the memory you're pointing to isn't owned by the crc
> > subsystem, but by the atomic commit worker. That could release the memory
> > (and it might get reused for something else meanwhile) before the crc
> > worker here is done with it.
> >
> >> + unsigned int x = state->src.x1 >> 16;
> >> + unsigned int y = state->src.y1 >> 16;
> >> + unsigned int height = drm_rect_height(>src) >> 16;
> >> + unsigned int width = drm_rect_width(>src) >> 16;
> >> + unsigned int cpp = fb->format->cpp[0];
> >> + unsigned int src_offset;
> >> + unsigned int size_byte = width * cpp;
> >> + void *vaddr = vkms_obj->vaddr;
> >> +
> >> + if (WARN_ON(!vaddr))
> >> + return crc;
> >> +
> >> + for (i = y; i < y + height; i++) {
> >> + src_offset = fb->offsets[0] + (i * fb->pitches[0]) + (x *
> >> cpp);
> >> + crc = crc32_le(crc, vaddr + src_offset, size_byte);
> >> + }
> >> +
> >> + return crc;
> >> +}
> >> +
> >> +void vkms_crc_work_handle(struct work_struct *work)
> >> +{
> >> + struct vkms_crtc_state *crtc_state = container_of(work,
> >> + struct vkms_crtc_state,
> >> + crc_workq);
> >> + struct drm_crtc *crtc = crtc_state->crtc;
> >> + struct drm_framebuffer *fb = (crtc->primary ? crtc->primary->fb :
> >> NULL);
> >
> > The drm_plane->fb pointer has similar issues: No locking (the only thing
> > that's really allowed to change this is the atomic commit worker for
> > atomic drivers, nothing else), so same issues with inconsistent data and
> > using possibly freed memory.
> >
> > On top Ville has a patch series to set drm_plane->fb == NULL for all
> > atomic drivers - it's really a leftover from the pre-atomic age and
> > doesn't fit for atomic.
>
> Ville just said that he pushed all that. Are you sure that you're on
> latest drm-tip? On latest drm-tip primary->fb should be always NULL
> for an atomic driver like vkms ... If you're not on latest drm-tip
> probably good idea to rebase everything.
> -Daniel
>
Ok I will work on that, thank you so much!
I did update my branch recently but then that resulted in networking
problems so I reverted the update. I will fix that issue first and then
work on the locking issues.
Thank you so much again!
Haneen
> >
> > On top of these issues for ->plane_state and ->fb we have a third issue:
> > The crc computation must be atomic wrt the generated vblank event for a
> > given atomic update. That means we must make sure that the crc we're
> > generating is exactly for the plane_state/fb that also issued the vblank
> > event. There's some atomic kms tests in igt that check this. More
> > concretely we need to make sure that:
> > - if the vblank code sees the new vblank event, then the crc _must_
> > compute the crc for the new configuration.
> > - if the vblank code does not yet see the new vblank event, then the crc
> > _must_ compute the crc for the old configuration.
> >
> > One
Re: [RFC 3/3] drm/vkms: Implement CRC debugfs API
On Thu, Jun 28, 2018 at 10:07 AM, Daniel Vetter wrote:
> On Thu, Jun 28, 2018 at 12:24:35AM +0300, Haneen Mohammed wrote:
>> Implement the .set_crc_source() callback.
>> Compute CRC using crc32 on the visible part of the framebuffer.
>> Use work_struct to compute and add CRC at the end of a vblank.
>>
>> Signed-off-by: Haneen Mohammed
>
> Ok locking review. I still don't have a clear idea how to best fix this,
> but oh well.
>
>> ---
>> drivers/gpu/drm/vkms/vkms_crtc.c | 76
>> drivers/gpu/drm/vkms/vkms_drv.h | 16 +++
>> 2 files changed, 92 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c
>> b/drivers/gpu/drm/vkms/vkms_crtc.c
>> index 73aae129c37d..d78934b76e33 100644
>> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
>> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
>> @@ -7,8 +7,78 @@
>> */
>>
>> #include "vkms_drv.h"
>> +#include
>> #include
>> #include
>> +#include
>> +
>> +uint32_t _vkms_get_crc(struct drm_framebuffer *fb)
>> +{
>> + struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
>> + struct vkms_gem_object *vkms_obj = container_of(gem_obj,
>> + struct vkms_gem_object,
>> + gem);
>> + u32 crc = 0;
>> + int i = 0;
>> + struct drm_plane_state *state = vkms_obj->plane_state;
>
> vkms_obj->plane_state is the first locking issue: You store the pointer
> without any locks or synchronization, which means the crc worker here
> could access is while it's being changed, resulting in an inconsistent
> crc. Note that the compiler/cpu is allowed to read a pointer multiple
> times if it's not protected by locking/barriers, so all kinds of funny
> stuff can happen here.
>
> Second issue is that the memory you're pointing to isn't owned by the crc
> subsystem, but by the atomic commit worker. That could release the memory
> (and it might get reused for something else meanwhile) before the crc
> worker here is done with it.
>
>> + unsigned int x = state->src.x1 >> 16;
>> + unsigned int y = state->src.y1 >> 16;
>> + unsigned int height = drm_rect_height(>src) >> 16;
>> + unsigned int width = drm_rect_width(>src) >> 16;
>> + unsigned int cpp = fb->format->cpp[0];
>> + unsigned int src_offset;
>> + unsigned int size_byte = width * cpp;
>> + void *vaddr = vkms_obj->vaddr;
>> +
>> + if (WARN_ON(!vaddr))
>> + return crc;
>> +
>> + for (i = y; i < y + height; i++) {
>> + src_offset = fb->offsets[0] + (i * fb->pitches[0]) + (x * cpp);
>> + crc = crc32_le(crc, vaddr + src_offset, size_byte);
>> + }
>> +
>> + return crc;
>> +}
>> +
>> +void vkms_crc_work_handle(struct work_struct *work)
>> +{
>> + struct vkms_crtc_state *crtc_state = container_of(work,
>> + struct vkms_crtc_state,
>> + crc_workq);
>> + struct drm_crtc *crtc = crtc_state->crtc;
>> + struct drm_framebuffer *fb = (crtc->primary ? crtc->primary->fb :
>> NULL);
>
> The drm_plane->fb pointer has similar issues: No locking (the only thing
> that's really allowed to change this is the atomic commit worker for
> atomic drivers, nothing else), so same issues with inconsistent data and
> using possibly freed memory.
>
> On top Ville has a patch series to set drm_plane->fb == NULL for all
> atomic drivers - it's really a leftover from the pre-atomic age and
> doesn't fit for atomic.
Ville just said that he pushed all that. Are you sure that you're on
latest drm-tip? On latest drm-tip primary->fb should be always NULL
for an atomic driver like vkms ... If you're not on latest drm-tip
probably good idea to rebase everything.
-Daniel
>
> On top of these issues for ->plane_state and ->fb we have a third issue:
> The crc computation must be atomic wrt the generated vblank event for a
> given atomic update. That means we must make sure that the crc we're
> generating is exactly for the plane_state/fb that also issued the vblank
> event. There's some atomic kms tests in igt that check this. More
> concretely we need to make sure that:
> - if the vblank code sees the new vblank event, then the crc _must_
> compute the crc for the new configuration.
> - if the vblank code does not yet see the new vblank event, then the crc
> _must_ compute the crc for the old configuration.
>
> One simple solution, but quite a bit of code to type would be.
>
> 1. add a spinlock to the vkms_crtc structure (probably need that first) to
> protect all the crc/vblank event stuff. Needs to be a spinlock since we
> must look at this from the hrtimer.
>
> 2. Create a new structure where you make a copy (not just pointers) of all
> the data the hrtimer needs. So src offset, vblank event, all that stuff.
> For the fb pointer you need to make sure it's properly reference counted
> (drm_framebuffer_get/put).
>
> 3. For every atomic update,
[RFC 3/3] drm/vkms: Implement CRC debugfs API
Implement the .set_crc_source() callback.
Compute CRC using crc32 on the visible part of the framebuffer.
Use work_struct to compute and add CRC at the end of a vblank.
Signed-off-by: Haneen Mohammed
---
drivers/gpu/drm/vkms/vkms_crtc.c | 76
drivers/gpu/drm/vkms/vkms_drv.h | 16 +++
2 files changed, 92 insertions(+)
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 73aae129c37d..d78934b76e33 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -7,8 +7,78 @@
*/
#include "vkms_drv.h"
+#include
#include
#include
+#include
+
+uint32_t _vkms_get_crc(struct drm_framebuffer *fb)
+{
+ struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
+ struct vkms_gem_object *vkms_obj = container_of(gem_obj,
+ struct vkms_gem_object,
+ gem);
+ u32 crc = 0;
+ int i = 0;
+ struct drm_plane_state *state = vkms_obj->plane_state;
+ unsigned int x = state->src.x1 >> 16;
+ unsigned int y = state->src.y1 >> 16;
+ unsigned int height = drm_rect_height(>src) >> 16;
+ unsigned int width = drm_rect_width(>src) >> 16;
+ unsigned int cpp = fb->format->cpp[0];
+ unsigned int src_offset;
+ unsigned int size_byte = width * cpp;
+ void *vaddr = vkms_obj->vaddr;
+
+ if (WARN_ON(!vaddr))
+ return crc;
+
+ for (i = y; i < y + height; i++) {
+ src_offset = fb->offsets[0] + (i * fb->pitches[0]) + (x * cpp);
+ crc = crc32_le(crc, vaddr + src_offset, size_byte);
+ }
+
+ return crc;
+}
+
+void vkms_crc_work_handle(struct work_struct *work)
+{
+ struct vkms_crtc_state *crtc_state = container_of(work,
+struct vkms_crtc_state,
+crc_workq);
+ struct drm_crtc *crtc = crtc_state->crtc;
+ struct drm_framebuffer *fb = (crtc->primary ? crtc->primary->fb : NULL);
+
+ if (crtc_state->crc_enabled && fb) {
+ u32 crc32 = _vkms_get_crc(fb);
+ unsigned int n_frame = drm_crtc_accurate_vblank_count(crtc);
+
+ drm_crtc_add_crc_entry(crtc, true, n_frame, );
+ }
+}
+
+static int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name,
+ size_t *values_cnt)
+{
+ struct drm_device *dev = crtc->dev;
+ struct vkms_device *vkms_dev = container_of(dev,
+ struct vkms_device,
+ drm);
+ struct vkms_crtc_state *crtc_state = _dev->output.crtc_state;
+
+ if (!src_name) {
+ crtc_state->crc_enabled = false;
+ } else if (strcmp(src_name, "auto") == 0) {
+ crtc_state->crc_enabled = true;
+ } else {
+ crtc_state->crc_enabled = false;
+ return -EINVAL;
+ }
+
+ *values_cnt = 1;
+
+ return 0;
+}
static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
{
@@ -23,6 +93,8 @@ static enum hrtimer_restart vkms_vblank_simulate(struct
hrtimer *timer)
if (!ret)
DRM_ERROR("vkms failure on handling vblank");
+ vkms_crc_work_handle(>crtc_state.crc_workq);
+
spin_lock_irqsave(>dev->event_lock, flags);
if (output->event) {
drm_crtc_send_vblank_event(crtc, output->event);
@@ -46,6 +118,9 @@ static int vkms_enable_vblank(struct drm_crtc *crtc)
out->period_ns = ktime_set(0, DEFAULT_VBLANK_NS);
hrtimer_start(>vblank_hrtimer, out->period_ns, HRTIMER_MODE_ABS);
+ out->crtc_state.crtc = crtc;
+ INIT_WORK(>crtc_state.crc_workq, vkms_crc_work_handle);
+
return 0;
}
@@ -65,6 +140,7 @@ static const struct drm_crtc_funcs vkms_crtc_funcs = {
.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
.enable_vblank = vkms_enable_vblank,
.disable_vblank = vkms_disable_vblank,
+ .set_crc_source = vkms_set_crc_source,
};
static int vkms_crtc_atomic_check(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 300e6a05d473..4a1458731dd7 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -22,6 +22,18 @@ static const u32 vkms_formats[] = {
DRM_FORMAT_XRGB,
};
+/**
+ * struct vkms_crtc_state
+ * @crtc: backpointer to the CRTC
+ * @crc_workq: worker that captures CRCs for each frame
+ * @crc_enabled: flag to test if crc is enabled
+ */
+struct vkms_crtc_state {
+ struct drm_crtc *crtc;
+ struct work_struct crc_workq;
+ bool crc_enabled;
+};
+
struct vkms_output {
struct drm_crtc crtc;
struct drm_encoder encoder;
@@ -29,6 +41,7 @@
Re: [RFC 3/3] drm/vkms: Implement CRC debugfs API
On Thu, Jun 28, 2018 at 10:07:34AM +0200, Daniel Vetter wrote:
> On Thu, Jun 28, 2018 at 12:24:35AM +0300, Haneen Mohammed wrote:
> > Implement the .set_crc_source() callback.
> > Compute CRC using crc32 on the visible part of the framebuffer.
> > Use work_struct to compute and add CRC at the end of a vblank.
> >
> > Signed-off-by: Haneen Mohammed
>
> Ok locking review. I still don't have a clear idea how to best fix this,
> but oh well.
>
> > ---
> > drivers/gpu/drm/vkms/vkms_crtc.c | 76
> > drivers/gpu/drm/vkms/vkms_drv.h | 16 +++
> > 2 files changed, 92 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c
> > b/drivers/gpu/drm/vkms/vkms_crtc.c
> > index 73aae129c37d..d78934b76e33 100644
> > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > @@ -7,8 +7,78 @@
> > */
> >
> > #include "vkms_drv.h"
> > +#include
> > #include
> > #include
> > +#include
> > +
> > +uint32_t _vkms_get_crc(struct drm_framebuffer *fb)
> > +{
> > + struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
> > + struct vkms_gem_object *vkms_obj = container_of(gem_obj,
> > + struct vkms_gem_object,
> > + gem);
> > + u32 crc = 0;
> > + int i = 0;
> > + struct drm_plane_state *state = vkms_obj->plane_state;
>
> vkms_obj->plane_state is the first locking issue: You store the pointer
> without any locks or synchronization, which means the crc worker here
> could access is while it's being changed, resulting in an inconsistent
> crc. Note that the compiler/cpu is allowed to read a pointer multiple
> times if it's not protected by locking/barriers, so all kinds of funny
> stuff can happen here.
>
> Second issue is that the memory you're pointing to isn't owned by the crc
> subsystem, but by the atomic commit worker. That could release the memory
> (and it might get reused for something else meanwhile) before the crc
> worker here is done with it.
>
> > + unsigned int x = state->src.x1 >> 16;
> > + unsigned int y = state->src.y1 >> 16;
> > + unsigned int height = drm_rect_height(>src) >> 16;
> > + unsigned int width = drm_rect_width(>src) >> 16;
> > + unsigned int cpp = fb->format->cpp[0];
> > + unsigned int src_offset;
> > + unsigned int size_byte = width * cpp;
> > + void *vaddr = vkms_obj->vaddr;
> > +
> > + if (WARN_ON(!vaddr))
> > + return crc;
> > +
> > + for (i = y; i < y + height; i++) {
> > + src_offset = fb->offsets[0] + (i * fb->pitches[0]) + (x * cpp);
> > + crc = crc32_le(crc, vaddr + src_offset, size_byte);
> > + }
> > +
> > + return crc;
> > +}
> > +
> > +void vkms_crc_work_handle(struct work_struct *work)
> > +{
> > + struct vkms_crtc_state *crtc_state = container_of(work,
> > +struct vkms_crtc_state,
> > +crc_workq);
> > + struct drm_crtc *crtc = crtc_state->crtc;
> > + struct drm_framebuffer *fb = (crtc->primary ? crtc->primary->fb : NULL);
>
> The drm_plane->fb pointer has similar issues: No locking (the only thing
> that's really allowed to change this is the atomic commit worker for
> atomic drivers, nothing else), so same issues with inconsistent data and
> using possibly freed memory.
>
> On top Ville has a patch series to set drm_plane->fb == NULL for all
> atomic drivers - it's really a leftover from the pre-atomic age and
> doesn't fit for atomic.
>
> On top of these issues for ->plane_state and ->fb we have a third issue:
> The crc computation must be atomic wrt the generated vblank event for a
> given atomic update. That means we must make sure that the crc we're
> generating is exactly for the plane_state/fb that also issued the vblank
> event. There's some atomic kms tests in igt that check this. More
> concretely we need to make sure that:
> - if the vblank code sees the new vblank event, then the crc _must_
> compute the crc for the new configuration.
> - if the vblank code does not yet see the new vblank event, then the crc
> _must_ compute the crc for the old configuration.
>
> One simple solution, but quite a bit of code to type would be.
>
> 1. add a spinlock to the vkms_crtc structure (probably need that first) to
> protect all the crc/vblank event stuff. Needs to be a spinlock since we
> must look at this from the hrtimer.
>
> 2. Create a new structure where you make a copy (not just pointers) of all
> the data the hrtimer needs. So src offset, vblank event, all that stuff.
> For the fb pointer you need to make sure it's properly reference counted
> (drm_framebuffer_get/put).
>
> 3. For every atomic update, allocate a new such structure and store it in
> the vkms_crtc structure (holding the spinlock while updating the pointer).
>
> 4. In the hrtimer grab that pointer and set it to NULL (again holding the
> spinlock), and then
Re: [RFC 3/3] drm/vkms: Implement CRC debugfs API
On Thu, Jun 28, 2018 at 12:24:35AM +0300, Haneen Mohammed wrote:
> Implement the .set_crc_source() callback.
> Compute CRC using crc32 on the visible part of the framebuffer.
> Use work_struct to compute and add CRC at the end of a vblank.
>
> Signed-off-by: Haneen Mohammed
Ok locking review. I still don't have a clear idea how to best fix this,
but oh well.
> ---
> drivers/gpu/drm/vkms/vkms_crtc.c | 76
> drivers/gpu/drm/vkms/vkms_drv.h | 16 +++
> 2 files changed, 92 insertions(+)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c
> b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 73aae129c37d..d78934b76e33 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -7,8 +7,78 @@
> */
>
> #include "vkms_drv.h"
> +#include
> #include
> #include
> +#include
> +
> +uint32_t _vkms_get_crc(struct drm_framebuffer *fb)
> +{
> + struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
> + struct vkms_gem_object *vkms_obj = container_of(gem_obj,
> + struct vkms_gem_object,
> + gem);
> + u32 crc = 0;
> + int i = 0;
> + struct drm_plane_state *state = vkms_obj->plane_state;
vkms_obj->plane_state is the first locking issue: You store the pointer
without any locks or synchronization, which means the crc worker here
could access is while it's being changed, resulting in an inconsistent
crc. Note that the compiler/cpu is allowed to read a pointer multiple
times if it's not protected by locking/barriers, so all kinds of funny
stuff can happen here.
Second issue is that the memory you're pointing to isn't owned by the crc
subsystem, but by the atomic commit worker. That could release the memory
(and it might get reused for something else meanwhile) before the crc
worker here is done with it.
> + unsigned int x = state->src.x1 >> 16;
> + unsigned int y = state->src.y1 >> 16;
> + unsigned int height = drm_rect_height(>src) >> 16;
> + unsigned int width = drm_rect_width(>src) >> 16;
> + unsigned int cpp = fb->format->cpp[0];
> + unsigned int src_offset;
> + unsigned int size_byte = width * cpp;
> + void *vaddr = vkms_obj->vaddr;
> +
> + if (WARN_ON(!vaddr))
> + return crc;
> +
> + for (i = y; i < y + height; i++) {
> + src_offset = fb->offsets[0] + (i * fb->pitches[0]) + (x * cpp);
> + crc = crc32_le(crc, vaddr + src_offset, size_byte);
> + }
> +
> + return crc;
> +}
> +
> +void vkms_crc_work_handle(struct work_struct *work)
> +{
> + struct vkms_crtc_state *crtc_state = container_of(work,
> + struct vkms_crtc_state,
> + crc_workq);
> + struct drm_crtc *crtc = crtc_state->crtc;
> + struct drm_framebuffer *fb = (crtc->primary ? crtc->primary->fb : NULL);
The drm_plane->fb pointer has similar issues: No locking (the only thing
that's really allowed to change this is the atomic commit worker for
atomic drivers, nothing else), so same issues with inconsistent data and
using possibly freed memory.
On top Ville has a patch series to set drm_plane->fb == NULL for all
atomic drivers - it's really a leftover from the pre-atomic age and
doesn't fit for atomic.
On top of these issues for ->plane_state and ->fb we have a third issue:
The crc computation must be atomic wrt the generated vblank event for a
given atomic update. That means we must make sure that the crc we're
generating is exactly for the plane_state/fb that also issued the vblank
event. There's some atomic kms tests in igt that check this. More
concretely we need to make sure that:
- if the vblank code sees the new vblank event, then the crc _must_
compute the crc for the new configuration.
- if the vblank code does not yet see the new vblank event, then the crc
_must_ compute the crc for the old configuration.
One simple solution, but quite a bit of code to type would be.
1. add a spinlock to the vkms_crtc structure (probably need that first) to
protect all the crc/vblank event stuff. Needs to be a spinlock since we
must look at this from the hrtimer.
2. Create a new structure where you make a copy (not just pointers) of all
the data the hrtimer needs. So src offset, vblank event, all that stuff.
For the fb pointer you need to make sure it's properly reference counted
(drm_framebuffer_get/put).
3. For every atomic update, allocate a new such structure and store it in
the vkms_crtc structure (holding the spinlock while updating the pointer).
4. In the hrtimer grab that pointer and set it to NULL (again holding the
spinlock), and then explicitly send out the vblank event (using
drm_crtc_send_vblank_event). And then pass that structure with everything
the crc worker needs to the crc worker.
I think this should work, mostly.
-Daniel
> +
> + if (crtc_state->crc_enabled &&
