Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1455759 > unrolled thread

Re: [PATCH v4 5/5] dma-buf/sync_file: only enable fence signalling on poll()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 5/5] dma-buf/sync_file: only enable fence signalling  on poll() Chris Wilson <chris@chris-wilson.co.uk> - 2016-08-03 13:50 +0200
    Re: [PATCH v4 5/5] dma-buf/sync_file: only enable fence signalling  on poll() Gustavo Padovan <gustavo@padovan.org> - 2016-08-04 23:20 +0200
      Re: [PATCH v4 5/5] dma-buf/sync_file: only enable fence signalling  on poll() Chris Wilson <chris@chris-wilson.co.uk> - 2016-08-04 23:40 +0200

#1455759 — Re: [PATCH v4 5/5] dma-buf/sync_file: only enable fence signalling on poll()

On Tue, Jul 12, 2016 at 03:08:45PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Signalling doesn't need to be enabled at sync_file creation, it is only
> required if userspace waiting the fence to signal through poll().
> 
> Thus we delay fence_add_callback() until poll is called. It only adds the
> callback the first time poll() is called. This avoid re-adding the same
> callback multiple times.
> 
> v2: rebase and update to work with new fence support for sync_file
> 
> v3: use atomic operation to set enabled and protect fence_add_callback()

There's actually a spare bit in fence->flags you can use for this.

#define POLL_ENABLED FENCE_FLAG_USER_BITS

if (test_bit(POLL_ENABLED, &sync_file->fence->flags))
	fence_remove_callback(sync_file->fence, &sync_file->cb);

...

if (!test_and_set_bit(POLL_ENABLED, &sync_file->fence->flags)) {
	if (fence_add_callback(sync_file->fence, &sync_file->cb,
			       fence_check_cb_func) < 0)
		wake_up_all(&sync_file->wq);
}

Saves adding a raw atomic.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[toc] | [next] | [standalone]

#1456724

2016-08-03 Chris Wilson <chris@chris-wilson.co.uk>:

> On Tue, Jul 12, 2016 at 03:08:45PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > Signalling doesn't need to be enabled at sync_file creation, it is only
> > required if userspace waiting the fence to signal through poll().
> > 
> > Thus we delay fence_add_callback() until poll is called. It only adds the
> > callback the first time poll() is called. This avoid re-adding the same
> > callback multiple times.
> > 
> > v2: rebase and update to work with new fence support for sync_file
> > 
> > v3: use atomic operation to set enabled and protect fence_add_callback()
> 
> There's actually a spare bit in fence->flags you can use for this.
> 
> #define POLL_ENABLED FENCE_FLAG_USER_BITS

Wouldn't it be better to add a new bit to fence_flags_bit?

	Gustavo

[toc] | [prev] | [next] | [standalone]

#1456744

On Thu, Aug 04, 2016 at 06:18:53PM -0300, Gustavo Padovan wrote:
> 2016-08-03 Chris Wilson <chris@chris-wilson.co.uk>:
> 
> > On Tue, Jul 12, 2016 at 03:08:45PM -0300, Gustavo Padovan wrote:
> > > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > > 
> > > Signalling doesn't need to be enabled at sync_file creation, it is only
> > > required if userspace waiting the fence to signal through poll().
> > > 
> > > Thus we delay fence_add_callback() until poll is called. It only adds the
> > > callback the first time poll() is called. This avoid re-adding the same
> > > callback multiple times.
> > > 
> > > v2: rebase and update to work with new fence support for sync_file
> > > 
> > > v3: use atomic operation to set enabled and protect fence_add_callback()
> > 
> > There's actually a spare bit in fence->flags you can use for this.
> > 
> > #define POLL_ENABLED FENCE_FLAG_USER_BITS
> 
> Wouldn't it be better to add a new bit to fence_flags_bit?

sync_file is a user of struct fence, so it should claim one of the bits
already reserved for users. Those reserved bits are meant only for the
owner of the fence, if we did indeed need to share that bit with other
consumers of the sync_file->fence_array then adding it to
fence_flags_bits make sense. I don't see any reason at present why it
should be anything other than a private bit to sync_file atm.

Promoting it later (from private to shared) would also not be an issue.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web