All of lore.kernel.org
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Haneen Mohammed <hamohammed.sa@gmail.com>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 03/10] drm/vkms: Rename vkms_output.state_lock to crc_lock
Date: Thu, 13 Jun 2019 09:48:46 +0200	[thread overview]
Message-ID: <20190613074846.GA23020@phenom.ffwll.local> (raw)
In-Reply-To: <CADKXj+5oBpMqu_SDvRB=j7Brav2_MXDccqAkN4UHr3Piwn66pQ@mail.gmail.com>

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

-- 
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-13  7:48 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
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 [this message]
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=20190613074846.GA23020@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 \
    --subject='Re: [PATCH 03/10] drm/vkms: Rename vkms_output.state_lock to crc_lock' \
    /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.