On Fri, Nov 15, 2019 at 12:29:38PM -0800, Matt Roper wrote:
> On Mon, Nov 11, 2019 at 12:50:24PM -0800, Lucas De Marchi wrote:
> > The current dsb API is not really prepared to handle multithread access.
> > I was debugging an issue that ended up fixed by commit a096883dda2c
> > ("drm/i915/dsb: Remove PIN_MAPPABLE from the DSB object VMA") and was
> > puzzled how these atomic operations were guaranteeing atomicity.
> >
> > if (atomic_add_return(1, &dsb->refcount) != 1)
> > return dsb;
> >
> > Thread A could still be initializing dsb struct (and even fail in the
> > middle) while thread B would take a reference and use it (even
> > derefencing a NULL cmd_buf).
> >
> > I don't think the atomic operations here will help much if this were
> > to support multithreaded scenario in future, so just remove them to
> > avoid confusion.
> >
> > Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
>
> Agreed; the synchronization here doesn't appear to make sense. But
> also I believe everywhere this would get called is on the atomic commit
> path and we already hold the CRTC lock at that point which will prevent
> concurrent threads calling this. So
Nonblocking commits are unlocked. And I'm thinking we should make
blocking commits unlocked as well. However commits for a specific
crtc are serialized so assuming this dsb stuff is per-crtc we should
probably be fine.
This whole refcount stuff seems a bit overkill tbh. We have a
fixed number of dsbs per pipe and fixed roles we could assign
to them. So I have a feeling all of this should just go away.
>
> Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
>
> > ---
> > drivers/gpu/drm/i915/display/intel_dsb.c | 10 +++++-----
> > drivers/gpu/drm/i915/display/intel_dsb.h | 2 +-
> > 2 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> > index d8ad5fe1efef..4feebbeb0b0c 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> > @@ -107,7 +107,7 @@ intel_dsb_get(struct intel_crtc *crtc)
> > if (!HAS_DSB(i915))
> > return dsb;
> >
> > - if (atomic_add_return(1, &dsb->refcount) != 1)
> > + if (++dsb->refcount != 1)
The usual pattern my brain is accustomed to is
if (ref++ == 0)
enable();
if (--ref == 0)
disabble();
> > return dsb;
> >
> > dsb->id = DSB1;
> > @@ -123,7 +123,7 @@ intel_dsb_get(struct intel_crtc *crtc)
> > if (IS_ERR(vma)) {
> > DRM_ERROR("Vma creation failed\n");
> > i915_gem_object_put(obj);
> > - atomic_dec(&dsb->refcount);
> > + dsb->refcount--;
> > goto err;
> > }
> >
> > @@ -132,7 +132,7 @@ intel_dsb_get(struct intel_crtc *crtc)
> > DRM_ERROR("Command buffer creation failed\n");
> > i915_vma_unpin_and_release(&vma, 0);
> > dsb->cmd_buf = NULL;
> > - atomic_dec(&dsb->refcount);
> > + dsb->refcount--;
> > goto err;
> > }
> > dsb->vma = vma;
> > @@ -158,10 +158,10 @@ void intel_dsb_put(struct intel_dsb *dsb)
> > if (!HAS_DSB(i915))
> > return;
> >
> > - if (WARN_ON(atomic_read(&dsb->refcount) == 0))
> > + if (WARN_ON(dsb->refcount == 0))
> > return;
> >
> > - if (atomic_dec_and_test(&dsb->refcount)) {
> > + if (--dsb->refcount == 0) {
> > i915_vma_unpin_and_release(&dsb->vma, I915_VMA_RELEASE_MAP);
> > dsb->cmd_buf = NULL;
> > dsb->free_pos = 0;
> > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
> > index 6f95c8e909e6..395ef9ce558e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dsb.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
> > @@ -22,7 +22,7 @@ enum dsb_id {
> > };
> >
> > struct intel_dsb {
> > - atomic_t refcount;
> > + long refcount;
> > enum dsb_id id;
> > u32 *cmd_buf;
> > struct i915_vma *vma;
> > --
> > 2.24.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Matt Roper
> Graphics Software Engineer
> VTT-OSGC Platform Enablement
> Intel Corporation
> (916) 356-2795
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
