[02/10] drm/vkms: Use spin_lock_irq in process context
diff mbox series

Message ID 20190606222751.32567-3-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • drm/vkms: rework crc worker
Related show

Commit Message

Daniel Vetter June 6, 2019, 10:27 p.m. UTC
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(-)

Comments

Rodrigo Siqueira June 12, 2019, 1:34 p.m. UTC | #1
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
>
Daniel Vetter June 12, 2019, 2:54 p.m. UTC | #2
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

Patch
diff mbox series

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;
 }