All of lore.kernel.org
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Cc: Haneen Mohammed <hamohammed.sa@gmail.com>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Shayenne Moura <shayenneluzmoura@gmail.com>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 02/10] drm/vkms: Use spin_lock_irq in process context
Date: Wed, 12 Jun 2019 16:54:25 +0200	[thread overview]
Message-ID: <20190612145425.GQ2458@phenom.ffwll.local> (raw)
In-Reply-To: <CADKXj+4HnVEoK7_RVVeicSgxgKe88zTG0HAUR-z1NJdtQs1i4g@mail.gmail.com>

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

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

  reply	other threads:[~2019-06-12 14:54 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-06 22:27 [PATCH 00/10] drm/vkms: rework crc worker Daniel Vetter
2019-06-06 22:27 ` [PATCH 01/10] drm/vkms: Fix crc worker races Daniel Vetter
2019-06-12 13:33   ` Rodrigo Siqueira
2019-06-12 14:48     ` Daniel Vetter
2019-06-18  2:39       ` Rodrigo Siqueira
2019-06-18  8:49         ` Daniel Vetter
2019-06-06 22:27 ` [PATCH 02/10] drm/vkms: Use spin_lock_irq in process context Daniel Vetter
2019-06-12 13:34   ` Rodrigo Siqueira
2019-06-12 14:54     ` Daniel Vetter [this message]
2019-06-06 22:27 ` [PATCH 03/10] drm/vkms: Rename vkms_output.state_lock to crc_lock Daniel Vetter
2019-06-12 13:38   ` Rodrigo Siqueira
2019-06-13  7:48     ` Daniel Vetter
2019-06-06 22:27 ` [PATCH 04/10] drm/vkms: Move format arrays to vkms_plane.c Daniel Vetter
2019-06-12 13:39   ` Rodrigo Siqueira
2019-06-19  2:12   ` Rodrigo Siqueira
2019-06-06 22:27 ` [PATCH 05/10] drm/vkms: Add our own commit_tail Daniel Vetter
2019-06-06 22:27 ` [PATCH 06/10] drm/vkms: flush crc workers earlier in commit flow Daniel Vetter
2019-06-12 13:42   ` Rodrigo Siqueira
2019-06-13  7:53     ` Daniel Vetter
2019-06-13  7:55       ` Daniel Vetter
2019-06-18  2:31       ` Rodrigo Siqueira
2019-06-06 22:27 ` [PATCH 07/10] drm/vkms: Dont flush crc worker when we change crc status Daniel Vetter
2019-06-19  2:17   ` Rodrigo Siqueira
2019-06-19  7:47     ` Daniel Vetter
2019-06-06 22:27 ` [PATCH 08/10] drm/vkms: No _irqsave within spin_lock_irq needed Daniel Vetter
2019-06-12 13:43   ` Rodrigo Siqueira
2019-06-06 22:27 ` [PATCH 09/10] drm/vkms: totally reworked crc data tracking Daniel Vetter
2019-06-12 13:46   ` Rodrigo Siqueira
2019-06-13  7:59     ` Daniel Vetter
2019-06-06 22:27 ` [PATCH 10/10] drm/vkms: No need for ->pages_lock in crc work anymore Daniel Vetter
2019-06-12 13:47   ` Rodrigo Siqueira
2019-06-12 13:28 ` [PATCH 00/10] drm/vkms: rework crc worker Rodrigo Siqueira
2019-06-12 14:42   ` Daniel Vetter
2019-06-18  2:49     ` Rodrigo Siqueira
2019-06-18  8:56       ` Daniel Vetter
2019-06-18 21:54         ` Rodrigo Siqueira
2019-06-18 22:06           ` Daniel Vetter
2019-06-18 22:07             ` Daniel Vetter
2019-06-18 22:25               ` Rodrigo Siqueira
2019-06-18 22:39                 ` Daniel Vetter
2019-06-26  1:44             ` Rodrigo Siqueira
2019-06-26  7:54               ` Daniel Vetter
2019-06-26 13:46                 ` Rodrigo Siqueira
2019-07-01  3:30                 ` Rodrigo Siqueira

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190612145425.GQ2458@phenom.ffwll.local \
    --to=daniel@ffwll.ch \
    --cc=daniel.vetter@ffwll.ch \
    --cc=daniel.vetter@intel.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=hamohammed.sa@gmail.com \
    --cc=rodrigosiqueiramelo@gmail.com \
    --cc=shayenneluzmoura@gmail.com \
    --subject='Re: [PATCH 02/10] drm/vkms: Use spin_lock_irq in process context' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.