| Message ID | 20190606222751.32567-4-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vkms: rework crc worker | expand |
On Thu, Jun 6, 2019 at 7:28 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > Plus add a comment about what it actually protects. It's very little. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > Cc: Haneen Mohammed <hamohammed.sa@gmail.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > --- > drivers/gpu/drm/vkms/vkms_crc.c | 4 ++-- > drivers/gpu/drm/vkms/vkms_crtc.c | 6 +++--- > drivers/gpu/drm/vkms/vkms_drv.h | 5 +++-- > 3 files changed, 8 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c > index 883e36fe7b6e..96806cd35ad4 100644 > --- a/drivers/gpu/drm/vkms/vkms_crc.c > +++ b/drivers/gpu/drm/vkms/vkms_crc.c > @@ -168,14 +168,14 @@ void vkms_crc_work_handle(struct work_struct *work) > u64 frame_start, frame_end; > bool crc_pending; > > - spin_lock_irq(&out->state_lock); > + spin_lock_irq(&out->crc_lock); > 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_irq(&out->state_lock); > + spin_unlock_irq(&out->crc_lock); > > /* > * We raced with the vblank hrtimer and previous work already computed > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c > index c727d8486e97..55b16d545fe7 100644 > --- a/drivers/gpu/drm/vkms/vkms_crtc.c > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c > @@ -29,7 +29,7 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) > /* update frame_start only if a queued vkms_crc_work_handle() > * has read the data > */ > - spin_lock(&output->state_lock); > + spin_lock(&output->crc_lock); > if (!state->crc_pending) > state->frame_start = frame; > else > @@ -37,7 +37,7 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) > state->frame_start, frame); > state->frame_end = frame; > state->crc_pending = true; > - spin_unlock(&output->state_lock); > + spin_unlock(&output->crc_lock); > > ret = queue_work(output->crc_workq, &state->crc_work); > if (!ret) > @@ -224,7 +224,7 @@ 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); > - spin_lock_init(&vkms_out->state_lock); > + spin_lock_init(&vkms_out->crc_lock); > > vkms_out->crc_workq = alloc_ordered_workqueue("vkms_crc_workq", 0); > if (!vkms_out->crc_workq) > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h > index 3c7e06b19efd..43d3f98289fe 100644 > --- a/drivers/gpu/drm/vkms/vkms_drv.h > +++ b/drivers/gpu/drm/vkms/vkms_drv.h > @@ -57,6 +57,7 @@ struct vkms_crtc_state { > struct drm_crtc_state base; > struct work_struct crc_work; > > + /* below three are protected by vkms_output.crc_lock */ > bool crc_pending; > u64 frame_start; > u64 frame_end; > @@ -74,8 +75,8 @@ struct vkms_output { > struct workqueue_struct *crc_workq; > /* protects concurrent access to crc_data */ > spinlock_t lock; It's not really specific to this patch, but after reviewing it, I was thinking about the use of the 'lock' field in the struct vkms_output. Do we really need it? It looks like that crc_lock just replaced it. Additionally, going a little bit deeper in the lock field at struct vkms_output, I was also asking myself: what critical area do we want to protect with this lock? After inspecting the use of this lock, I noticed two different places that use it: 1. In the function vkms_vblank_simulate() Note that we cover a vast area with ‘output->lock’. IMHO we just need to protect the state variable (line “state = output->crc_state;”) and we can also use crc_lock for this case. Make sense? 2. In the function vkms_crtc_atomic_begin() We also hold output->lock in the vkms_crtc_atomic_begin() and just release it at vkms_crtc_atomic_flush(). Do we still need it? > - /* protects concurrent access to crtc_state */ > - spinlock_t state_lock; > + > + spinlock_t crc_lock; > }; Maybe add a kernel doc on top of crc_lock? > > struct vkms_device { > -- > 2.20.1 >
On Wed, Jun 12, 2019 at 10:38:23AM -0300, Rodrigo Siqueira wrote: > On Thu, Jun 6, 2019 at 7:28 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > > Plus add a comment about what it actually protects. It's very little. > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > > Cc: Haneen Mohammed <hamohammed.sa@gmail.com> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > --- > > drivers/gpu/drm/vkms/vkms_crc.c | 4 ++-- > > drivers/gpu/drm/vkms/vkms_crtc.c | 6 +++--- > > drivers/gpu/drm/vkms/vkms_drv.h | 5 +++-- > > 3 files changed, 8 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c > > index 883e36fe7b6e..96806cd35ad4 100644 > > --- a/drivers/gpu/drm/vkms/vkms_crc.c > > +++ b/drivers/gpu/drm/vkms/vkms_crc.c > > @@ -168,14 +168,14 @@ void vkms_crc_work_handle(struct work_struct *work) > > u64 frame_start, frame_end; > > bool crc_pending; > > > > - spin_lock_irq(&out->state_lock); > > + spin_lock_irq(&out->crc_lock); > > 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_irq(&out->state_lock); > > + spin_unlock_irq(&out->crc_lock); > > > > /* > > * We raced with the vblank hrtimer and previous work already computed > > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c > > index c727d8486e97..55b16d545fe7 100644 > > --- a/drivers/gpu/drm/vkms/vkms_crtc.c > > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c > > @@ -29,7 +29,7 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) > > /* update frame_start only if a queued vkms_crc_work_handle() > > * has read the data > > */ > > - spin_lock(&output->state_lock); > > + spin_lock(&output->crc_lock); > > if (!state->crc_pending) > > state->frame_start = frame; > > else > > @@ -37,7 +37,7 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) > > state->frame_start, frame); > > state->frame_end = frame; > > state->crc_pending = true; > > - spin_unlock(&output->state_lock); > > + spin_unlock(&output->crc_lock); > > > > ret = queue_work(output->crc_workq, &state->crc_work); > > if (!ret) > > @@ -224,7 +224,7 @@ 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); > > - spin_lock_init(&vkms_out->state_lock); > > + spin_lock_init(&vkms_out->crc_lock); > > > > vkms_out->crc_workq = alloc_ordered_workqueue("vkms_crc_workq", 0); > > if (!vkms_out->crc_workq) > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h > > index 3c7e06b19efd..43d3f98289fe 100644 > > --- a/drivers/gpu/drm/vkms/vkms_drv.h > > +++ b/drivers/gpu/drm/vkms/vkms_drv.h > > @@ -57,6 +57,7 @@ struct vkms_crtc_state { > > struct drm_crtc_state base; > > struct work_struct crc_work; > > > > + /* below three are protected by vkms_output.crc_lock */ > > bool crc_pending; > > u64 frame_start; > > u64 frame_end; > > @@ -74,8 +75,8 @@ struct vkms_output { > > struct workqueue_struct *crc_workq; > > /* protects concurrent access to crc_data */ > > spinlock_t lock; > > It's not really specific to this patch, but after reviewing it, I was > thinking about the use of the 'lock' field in the struct vkms_output. > Do we really need it? It looks like that crc_lock just replaced it. Yeah they're a bit redundant, but the other way round. crc_lock is per vkms_output_state, and we constantly recreate that one. So if we'd want to remove one of those locks, that's the one that needs to go. Only vkms_output->lock is global. I figured not something we absolutely have to do right away :-) > Additionally, going a little bit deeper in the lock field at struct > vkms_output, I was also asking myself: what critical area do we want > to protect with this lock? After inspecting the use of this lock, I > noticed two different places that use it: > > 1. In the function vkms_vblank_simulate() > > Note that we cover a vast area with ‘output->lock’. IMHO we just need > to protect the state variable (line “state = output->crc_state;”) and > we can also use crc_lock for this case. Make sense? Hm very tricky, but good point. Going through that entire function, as it is after my patch series: - hrtimer_forward_now: I think with all the patches to fix the ordering it should be safe to take that out of the vkms_output->lock. in the get_vblank_timestamp function we also don't take the lock, so really doesn't protect anything. - drm_crtc_handle_vblank: This must be protected by output->lock to avoid races against drm_crtc_arm_vblank. - state = output->crc_state: Obviously needs the lock. - lifetime of state memory, i.e. what guarantees no one calls kfree on it before we're done. We already rely on this not disappearing until the worker has finished (using the flush_work before we tear down old state structures), but moving the queue_work out from under the state->lock protection might open up new race windows. I think from a logic pov it's all fine: We wait for vblank to signal, which means after that point any queue_work will be for the next state, not the one we're about to free, in vkms_atomic_commit_tail. Well the freeing is done in drm_atomic_helper.c which calls commit_tail. But we might be missing some barriers in drm_vblank.c. tldr; I think we can move the critical section down a bit, but until drm_vblank.c is fully audited we need to keep the bottom part as-is I think. > 2. In the function vkms_crtc_atomic_begin() > > We also hold output->lock in the vkms_crtc_atomic_begin() and just > release it at vkms_crtc_atomic_flush(). Do we still need it? > > > - /* protects concurrent access to crtc_state */ > > - spinlock_t state_lock; > > + > > + spinlock_t crc_lock; > > }; > > Maybe add a kernel doc on top of crc_lock? Atm we have 0 kerneldoc for vkms structures. I planned to fix that once this series has landed (well maybe need a bit more work still before it makes sense to start typing docs). -Daniel > > > > > struct vkms_device { > > -- > > 2.20.1 > > > > > -- > > Rodrigo Siqueira > https://siqueira.tech
diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c index 883e36fe7b6e..96806cd35ad4 100644 --- a/drivers/gpu/drm/vkms/vkms_crc.c +++ b/drivers/gpu/drm/vkms/vkms_crc.c @@ -168,14 +168,14 @@ void vkms_crc_work_handle(struct work_struct *work) u64 frame_start, frame_end; bool crc_pending; - spin_lock_irq(&out->state_lock); + spin_lock_irq(&out->crc_lock); 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_irq(&out->state_lock); + spin_unlock_irq(&out->crc_lock); /* * We raced with the vblank hrtimer and previous work already computed diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c index c727d8486e97..55b16d545fe7 100644 --- a/drivers/gpu/drm/vkms/vkms_crtc.c +++ b/drivers/gpu/drm/vkms/vkms_crtc.c @@ -29,7 +29,7 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) /* update frame_start only if a queued vkms_crc_work_handle() * has read the data */ - spin_lock(&output->state_lock); + spin_lock(&output->crc_lock); if (!state->crc_pending) state->frame_start = frame; else @@ -37,7 +37,7 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) state->frame_start, frame); state->frame_end = frame; state->crc_pending = true; - spin_unlock(&output->state_lock); + spin_unlock(&output->crc_lock); ret = queue_work(output->crc_workq, &state->crc_work); if (!ret) @@ -224,7 +224,7 @@ 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); - spin_lock_init(&vkms_out->state_lock); + spin_lock_init(&vkms_out->crc_lock); vkms_out->crc_workq = alloc_ordered_workqueue("vkms_crc_workq", 0); if (!vkms_out->crc_workq) diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index 3c7e06b19efd..43d3f98289fe 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.h +++ b/drivers/gpu/drm/vkms/vkms_drv.h @@ -57,6 +57,7 @@ struct vkms_crtc_state { struct drm_crtc_state base; struct work_struct crc_work; + /* below three are protected by vkms_output.crc_lock */ bool crc_pending; u64 frame_start; u64 frame_end; @@ -74,8 +75,8 @@ struct vkms_output { struct workqueue_struct *crc_workq; /* protects concurrent access to crc_data */ spinlock_t lock; - /* protects concurrent access to crtc_state */ - spinlock_t state_lock; + + spinlock_t crc_lock; }; struct vkms_device {
Plus add a comment about what it actually protects. It's very little. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> Cc: Haneen Mohammed <hamohammed.sa@gmail.com> Cc: Daniel Vetter <daniel@ffwll.ch> --- drivers/gpu/drm/vkms/vkms_crc.c | 4 ++-- drivers/gpu/drm/vkms/vkms_crtc.c | 6 +++--- drivers/gpu/drm/vkms/vkms_drv.h | 5 +++-- 3 files changed, 8 insertions(+), 7 deletions(-)