| Message ID | 20190606222751.32567-3-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: > > The worker is always in process context, no need for the _irqsafe > version. Same for the set_source callback, that's only called from the > debugfs handler in a syscall. > > Cc: Shayenne Moura <shayenneluzmoura@gmail.com> > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Haneen Mohammed <hamohammed.sa@gmail.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > --- > drivers/gpu/drm/vkms/vkms_crc.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c > index 66603da634fe..883e36fe7b6e 100644 > --- a/drivers/gpu/drm/vkms/vkms_crc.c > +++ b/drivers/gpu/drm/vkms/vkms_crc.c > @@ -167,16 +167,15 @@ void vkms_crc_work_handle(struct work_struct *work) > u32 crc32 = 0; > u64 frame_start, frame_end; > bool crc_pending; > - unsigned long flags; > > - spin_lock_irqsave(&out->state_lock, flags); > + spin_lock_irq(&out->state_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_irqrestore(&out->state_lock, flags); > + spin_unlock_irq(&out->state_lock); > > /* > * We raced with the vblank hrtimer and previous work already computed > @@ -246,7 +245,6 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name) > { > struct vkms_output *out = drm_crtc_to_vkms_output(crtc); > bool enabled = false; > - unsigned long flags; > int ret = 0; > > ret = vkms_crc_parse_source(src_name, &enabled); > @@ -254,9 +252,9 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name) > /* make sure nothing is scheduled on crtc workq */ > flush_workqueue(out->crc_workq); > > - spin_lock_irqsave(&out->lock, flags); > + spin_lock_irq(&out->lock); > out->crc_enabled = enabled; > - spin_unlock_irqrestore(&out->lock, flags); I was wondering if we could use atomic_t for crc_enabled and avoid this sort of lock. I did not try to change the data type; this is just an idea. Reviewed-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> Tested-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > + spin_unlock_irq(&out->lock); > > return ret; > } > -- > 2.20.1 >
On Wed, Jun 12, 2019 at 10:34:55AM -0300, Rodrigo Siqueira wrote: > On Thu, Jun 6, 2019 at 7:28 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > > The worker is always in process context, no need for the _irqsafe > > version. Same for the set_source callback, that's only called from the > > debugfs handler in a syscall. > > > > Cc: Shayenne Moura <shayenneluzmoura@gmail.com> > > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Haneen Mohammed <hamohammed.sa@gmail.com> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > --- > > drivers/gpu/drm/vkms/vkms_crc.c | 10 ++++------ > > 1 file changed, 4 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c > > index 66603da634fe..883e36fe7b6e 100644 > > --- a/drivers/gpu/drm/vkms/vkms_crc.c > > +++ b/drivers/gpu/drm/vkms/vkms_crc.c > > @@ -167,16 +167,15 @@ void vkms_crc_work_handle(struct work_struct *work) > > u32 crc32 = 0; > > u64 frame_start, frame_end; > > bool crc_pending; > > - unsigned long flags; > > > > - spin_lock_irqsave(&out->state_lock, flags); > > + spin_lock_irq(&out->state_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_irqrestore(&out->state_lock, flags); > > + spin_unlock_irq(&out->state_lock); > > > > /* > > * We raced with the vblank hrtimer and previous work already computed > > @@ -246,7 +245,6 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name) > > { > > struct vkms_output *out = drm_crtc_to_vkms_output(crtc); > > bool enabled = false; > > - unsigned long flags; > > int ret = 0; > > > > ret = vkms_crc_parse_source(src_name, &enabled); > > @@ -254,9 +252,9 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name) > > /* make sure nothing is scheduled on crtc workq */ > > flush_workqueue(out->crc_workq); > > > > - spin_lock_irqsave(&out->lock, flags); > > + spin_lock_irq(&out->lock); > > out->crc_enabled = enabled; > > - spin_unlock_irqrestore(&out->lock, flags); > > I was wondering if we could use atomic_t for crc_enabled and avoid > this sort of lock. I did not try to change the data type; this is just > an idea. tldr; atomic_t does not do what you think it does. rule of thumb is you can use it for reference counting and statistics book-keeping, but nothing else. The long explanation is that atomic_t in the linux kernel does not have barriers (unlike atomic values in almost all other language), they are weakly ordered. If you want to use them for logic (like here with this bool) you need to think very carefully about barriers, document those barriers, proof you got it all right, all for maybe a tiny speed-up (spinlocks are extremely well optimized). In almost all cases that's not worth it, and fairly often you end up with more atomic operations and so overall slower code. btw for this reasons atomic_t is wrapped as refcount_t for those cases where it's safe to use (plus the code is even more optimized for the refcount use-case). Except for statistics (like how many crc did we compute) you shouldn't ever use atomic_t, at least as a good rule of thumb. -Daniel > > Reviewed-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > Tested-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > > > + spin_unlock_irq(&out->lock); > > > > return ret; > > } > > -- > > 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 66603da634fe..883e36fe7b6e 100644 --- a/drivers/gpu/drm/vkms/vkms_crc.c +++ b/drivers/gpu/drm/vkms/vkms_crc.c @@ -167,16 +167,15 @@ void vkms_crc_work_handle(struct work_struct *work) u32 crc32 = 0; u64 frame_start, frame_end; bool crc_pending; - unsigned long flags; - spin_lock_irqsave(&out->state_lock, flags); + spin_lock_irq(&out->state_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_irqrestore(&out->state_lock, flags); + spin_unlock_irq(&out->state_lock); /* * We raced with the vblank hrtimer and previous work already computed @@ -246,7 +245,6 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name) { struct vkms_output *out = drm_crtc_to_vkms_output(crtc); bool enabled = false; - unsigned long flags; int ret = 0; ret = vkms_crc_parse_source(src_name, &enabled); @@ -254,9 +252,9 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name) /* make sure nothing is scheduled on crtc workq */ flush_workqueue(out->crc_workq); - spin_lock_irqsave(&out->lock, flags); + spin_lock_irq(&out->lock); out->crc_enabled = enabled; - spin_unlock_irqrestore(&out->lock, flags); + spin_unlock_irq(&out->lock); return ret; }
The worker is always in process context, no need for the _irqsafe version. Same for the set_source callback, that's only called from the debugfs handler in a syscall. Cc: Shayenne Moura <shayenneluzmoura@gmail.com> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Haneen Mohammed <hamohammed.sa@gmail.com> Cc: Daniel Vetter <daniel@ffwll.ch> --- drivers/gpu/drm/vkms/vkms_crc.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)