Thanks for this patch! It looks good for me.
Reviewed-by: Rodrigo Siqueira <rodrigosiqueiramelo@xxxxxxxxx>
On 07/19, Daniel Vetter wrote:
> We can reduce the critical section in vkms_vblank_simulate under
> output->lock quite a lot:
>
> - hrtimer_forward_now just needs to be ordered correctly wrt
> drm_crtc_handle_vblank. We already access the hrtimer timestamp
> without locks. While auditing that I noticed that we don't correctly
> annotate the read there, so sprinkle a READ_ONCE to make sure the
> compiler doesn't do anything foolish.
>
> - drm_crtc_handle_vblank must stay under the lock to avoid races with
> drm_crtc_arm_vblank_event.
>
> - The access to vkms_ouptut->crc_state also must stay under the lock.
>
> - next problem is making sure the output->state structure doesn't get
> freed too early. First we rely on a given hrtimer being serialized:
> If we call drm_crtc_handle_vblank, then we are guaranteed that the
> previous call to vkms_vblank_simulate has completed. The other side
> of the coin is that the atomic updates waits for the vblank to
> happen before it releases the old state. Both taken together means
> that by the time the atomic update releases the old state, the
> hrtimer won't access it anymore (it might be accessing the new state
> at the same time, but that's ok).
>
> - state is invariant, except the few fields separate protected by
> state->crc_lock. So no need to hold the lock for that.
>
> - finally the queue_work. We need to make sure there's no races with
> the flush_work, i.e. when we call flush_work we need to guarantee
> that the hrtimer can't requeue the work again. This is guaranteed by
> the same vblank/hrtimer ordering guarantees like the reasoning above
> why state won't be freed too early: flush_work on the old state is
> called after wait_for_flip_done in the atomic commit code.
>
> Therefore we can also move everything after the output->crc_state out
> of the critical section.
>
> Motivated by suggestions from Rodrigo.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@xxxxxxxxx>
> Cc: Haneen Mohammed <hamohammed.sa@xxxxxxxxx>
> Cc: Daniel Vetter <daniel@xxxxxxxx>
> ---
> drivers/gpu/drm/vkms/vkms_crtc.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 927dafaebc76..74f703b8d22a 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -16,17 +16,18 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> u64 ret_overrun;
> bool ret;
>
> - spin_lock(&output->lock);
> -
> ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
> output->period_ns);
> WARN_ON(ret_overrun != 1);
>
> + spin_lock(&output->lock);
> ret = drm_crtc_handle_vblank(crtc);
> if (!ret)
> DRM_ERROR("vkms failure on handling vblank");
>
> state = output->composer_state;
> + spin_unlock(&output->lock);
> +
> if (state && output->composer_enabled) {
> u64 frame = drm_crtc_accurate_vblank_count(crtc);
>
> @@ -48,8 +49,6 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> DRM_DEBUG_DRIVER("Composer worker already queued\n");
> }
>
> - spin_unlock(&output->lock);
> -
> return HRTIMER_RESTART;
> }
>
> @@ -85,7 +84,7 @@ bool vkms_get_vblank_timestamp(struct drm_device *dev, unsigned int pipe,
> struct vkms_output *output = &vkmsdev->output;
> struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>
> - *vblank_time = output->vblank_hrtimer.node.expires;
> + *vblank_time = READ_ONCE(output->vblank_hrtimer.node.expires);
>
> if (WARN_ON(*vblank_time == vblank->time))
> return true;
> --
> 2.22.0
>
--
Rodrigo Siqueira
Software Engineer, Advanced Micro Devices (AMD)
https://siqueira.tech
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
