On Tue, Sep 03, 2019 at 08:50:29AM -0400, Rodrigo Siqueira wrote:
> Thanks for this patch! It looks good for me.
>
> Reviewed-by: Rodrigo Siqueira <rodrigosiqueiram...@gmail.com>
Thanks for taking a look at all this, entire series merged. With the r-b
from Ville on patch 1, but I'm happy to further discuss your questions.
Plus I augmented the commit message a bit for patch 1 to explain the
missed vblank story better.
-Daniel
>
> 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.vet...@intel.com>
> > Cc: Rodrigo Siqueira <rodrigosiqueiram...@gmail.com>
> > Cc: Haneen Mohammed <hamohammed...@gmail.com>
> > Cc: Daniel Vetter <dan...@ffwll.ch>
> > ---
> > 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
