Re: [PATCH 1/2] drm/i915/dsb: remove atomic operations

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@intel.com>
> 
> 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@intel.com>
> 
> > ---
> >  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@lists.freedesktop.org
> > 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx