Commit 18d0952a authored by Daniel Vetter's avatar Daniel Vetter Committed by Rodrigo Siqueira
Browse files

drm/vkms: Fix crc worker races 



The issue we have is that the crc worker might fall behind. We've
tried to handle this by tracking both the earliest frame for which it
still needs to compute a crc, and the last one. Plus when the
crtc_state changes, we have a new work item, which are all run in
order due to the ordered workqueue we allocate for each vkms crtc.

Trouble is there's been a few small issues in the current code:
- we need to capture frame_end in the vblank hrtimer, not in the
  worker. The worker might run much later, and then we generate a lot
  of crc for which there's already a different worker queued up.
- frame number might be 0, so create a new crc_pending boolean to
  track this without confusion.
- we need to atomically grab frame_start/end and clear it, so do that
  all in one go. This is not going to create a new race, because if we
  race with the hrtimer then our work will be re-run.
- only race that can happen is the following:
  1. worker starts
  2. hrtimer runs and updates frame_end
  3. worker grabs frame_start/end, already reading the new frame_end,
  and clears crc_pending
  4. hrtimer calls queue_work()
  5. worker completes
  6. worker gets  re-run, crc_pending is false
  Explain this case a bit better by rewording the comment.

v2: Demote warning level output to debug when we fail to requeue, this
is expected under high load when the crc worker can't quite keep up.

Cc: Shayenne Moura <shayenneluzmoura@gmail.com>
Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: default avatarDaniel Vetter <daniel.vetter@intel.com>
Reviewed-by: default avatarRodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Tested-by: default avatarRodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Signed-off-by: default avatarRodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20190606222751.32567-2-daniel.vetter@ffwll.ch
parent c1a495a5

drivers/gpu/drm/vkms/vkms_crc.c

+13 −14
Original line number Diff line number Diff line
@@ -166,16 +166,24 @@ void vkms_crc_work_handle(struct work_struct *work) 
	struct drm_plane *plane;

	u32 crc32 = 0;

	u64 frame_start, frame_end;

	bool crc_pending;

	unsigned long flags;



	spin_lock_irqsave(&out->state_lock, flags);

	frame_start = crtc_state->frame_start;

	frame_end = crtc_state->frame_end;

	crc_pending = crtc_state->crc_pending;

	crtc_state->frame_start = 0;

	crtc_state->frame_end = 0;

	crtc_state->crc_pending = false;

	spin_unlock_irqrestore(&out->state_lock, flags);



	/* _vblank_handle() hasn't updated frame_start yet */

	if (!frame_start || frame_start == frame_end)

		goto out;

	/*

	 * We raced with the vblank hrtimer and previous work already computed

	 * the crc, nothing to do.

	 */

	if (!crc_pending)

		return;



	drm_for_each_plane(plane, &vdev->drm) {

		struct vkms_plane_state *vplane_state;
@@ -196,20 +204,11 @@ void vkms_crc_work_handle(struct work_struct *work) 
	if (primary_crc)

		crc32 = _vkms_get_crc(primary_crc, cursor_crc);



	frame_end = drm_crtc_accurate_vblank_count(crtc);



	/* queue_work can fail to schedule crc_work; add crc for

	 * missing frames

	/*

	 * The worker can fall behind the vblank hrtimer, make sure we catch up.

	 */

	while (frame_start <= frame_end)

		drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32);



out:

	/* to avoid using the same value for frame number again */

	spin_lock_irqsave(&out->state_lock, flags);

	crtc_state->frame_end = frame_end;

	crtc_state->frame_start = 0;

	spin_unlock_irqrestore(&out->state_lock, flags);

}



static const char * const pipe_crc_sources[] = {"auto"};

drivers/gpu/drm/vkms/vkms_crtc.c

+7 −2
Original line number Diff line number Diff line
@@ -30,13 +30,18 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) 
		 * has read the data

		 */

		spin_lock(&output->state_lock);

		if (!state->frame_start)

		if (!state->crc_pending)

			state->frame_start = frame;

		else

			DRM_DEBUG_DRIVER("crc worker falling behind, frame_start: %llu, frame_end: %llu\n",

					 state->frame_start, frame);

		state->frame_end = frame;

		state->crc_pending = true;

		spin_unlock(&output->state_lock);



		ret = queue_work(output->crc_workq, &state->crc_work);

		if (!ret)

			DRM_WARN("failed to queue vkms_crc_work_handle");

			DRM_DEBUG_DRIVER("vkms_crc_work_handle already queued\n");

	}



	spin_unlock(&output->lock);

drivers/gpu/drm/vkms/vkms_drv.h

+2 −0
Original line number Diff line number Diff line
@@ -48,6 +48,8 @@ struct vkms_plane_state { 
struct vkms_crtc_state {

	struct drm_crtc_state base;

	struct work_struct crc_work;



	bool crc_pending;

	u64 frame_start;

	u64 frame_end;

};

CRA Git | Maintained and supported by SUSTech CRA and CCSE