[Intel-gfx] [PATCH 1/2] drm/i915/dsb: remove atomic operations

[Lucas De Marchi]

On Fri, Nov 15, 2019 at 11:09:43PM +0200, Ville Syrjälä wrote:
>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 at 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

I agree

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

I think this was actually thought to protect reentrance rather than
concurrency, i.e. multiple calls to intel_dsb_get() before the _put().

>
>>
>> Reviewed-by: Matt Roper <matthew.d.roper at 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();

ok

thanks
Lucas De Marchi

>
>> >  		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 at 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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>-- 
>Ville Syrjälä
>Intel