subject:"Re\: \[PATCH v5 4\/4\] drm\/fence\: add out\-fences support"

Re: [PATCH v5 4/4] drm/fence: add out-fences support

2016-10-21 Thread Brian Starkey


Hi,

Sorry, I hit another couple of bugs that originated from my hastebin
patch - see below.

On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:

From: Gustavo Padovan 

Support DRM out-fences by creating a sync_file with a fence for each CRTC
that sets the OUT_FENCE_PTR property.

We use the out_fence pointer received in the OUT_FENCE_PTR prop to send
the sync_file fd back to userspace.

The sync_file and fd are allocated/created before commit, but the
fd_install operation only happens after we know that commit succeed.

In case of error userspace should receive a fd number of -1.

v2: Comment by Rob Clark:
- Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here.

   Comment by Daniel Vetter:
- Add clean up code for out_fences

v3: Comments by Daniel Vetter:
- create DRM_MODE_ATOMIC_EVENT_MASK
- userspace should fill out_fences_ptr with the crtc_ids for which
it wants fences back.

v4: Create OUT_FENCE_PTR properties and remove old approach.

Signed-off-by: Gustavo Padovan 
---
drivers/gpu/drm/drm_atomic.c | 116 ++-
drivers/gpu/drm/drm_crtc.c   |   8 +++
include/drm/drm_crtc.h   |  19 +++
3 files changed, 119 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index b3284b2..07775fc 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -490,6 +490,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
);
state->color_mgmt_changed |= replaced;
return ret;
+   } else if (property == config->prop_out_fence_ptr) {
+   state->out_fence_ptr = u64_to_user_ptr(val);
} else if (crtc->funcs->atomic_set_property)
return crtc->funcs->atomic_set_property(crtc, state, property, 
val);
else
@@ -532,6 +534,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
*val = (state->ctm) ? state->ctm->base.id : 0;
else if (property == config->gamma_lut_property)
*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
+   else if (property == config->prop_out_fence_ptr)
+   *val = 0;
else if (crtc->funcs->atomic_get_property)
return crtc->funcs->atomic_get_property(crtc, state, property, 
val);
else
@@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
 */

static struct drm_pending_vblank_event *create_vblank_event(
-   struct drm_device *dev, struct drm_file *file_priv,
-   struct fence *fence, uint64_t user_data)
+   struct drm_device *dev, uint64_t user_data)
{
struct drm_pending_vblank_event *e = NULL;
-   int ret;

e = kzalloc(sizeof *e, GFP_KERNEL);
if (!e)
@@ -1488,17 +1490,6 @@ static struct drm_pending_vblank_event 
*create_vblank_event(
e->event.base.length = sizeof(e->event);
e->event.user_data = user_data;

-   if (file_priv) {
-   ret = drm_event_reserve_init(dev, file_priv, >base,
->event.base);
-   if (ret) {
-   kfree(e);
-   return NULL;
-   }
-   }
-
-   e->base.fence = fence;
-
return e;
}

@@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
}
EXPORT_SYMBOL(drm_atomic_clean_old_fb);

+static int crtc_setup_out_fence(struct drm_crtc *crtc,
+   struct drm_crtc_state *crtc_state,
+   struct drm_out_fence_state *fence_state)
+{
+   struct fence *fence;
+
+   fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
+   if (fence_state->fd < 0) {
+   return fence_state->fd;
+   }
+
+   fence_state->out_fence_ptr = crtc_state->out_fence_ptr;
+   crtc_state->out_fence_ptr = NULL;
+
+   if (put_user(fence_state->fd, fence_state->out_fence_ptr))
+   return -EFAULT;
+
+   fence = kzalloc(sizeof(*fence), GFP_KERNEL);
+   if (!fence)
+   return -ENOMEM;
+
+   fence_init(fence, _crtc_fence_ops, >fence_lock,
+  crtc->fence_context, ++crtc->fence_seqno);
+
+   fence_state->sync_file = sync_file_create(fence);
+   if(!fence_state->sync_file) {
+   fence_put(fence);
+   return -ENOMEM;
+   }
+
+   crtc_state->event->base.fence = fence_get(fence);
+   return 0;
+}
+
int drm_mode_atomic_ioctl(struct drm_device *dev,
  void *data, struct drm_file *file_priv)
{
@@ -1617,9 +1642,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
struct drm_plane *plane;
struct drm_crtc *crtc;
struct drm_crtc_state *crtc_state;
+   struct drm_out_fence_state *fence_state;
unsigned

Re: [PATCH v5 4/4] drm/fence: add out-fences support

2016-10-21 Thread Brian Starkey


Hi,

Sorry, I hit another couple of bugs that originated from my hastebin
patch - see below.

On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:

From: Gustavo Padovan 

Support DRM out-fences by creating a sync_file with a fence for each CRTC
that sets the OUT_FENCE_PTR property.

We use the out_fence pointer received in the OUT_FENCE_PTR prop to send
the sync_file fd back to userspace.

The sync_file and fd are allocated/created before commit, but the
fd_install operation only happens after we know that commit succeed.

In case of error userspace should receive a fd number of -1.

v2: Comment by Rob Clark:
- Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here.

   Comment by Daniel Vetter:
- Add clean up code for out_fences

v3: Comments by Daniel Vetter:
- create DRM_MODE_ATOMIC_EVENT_MASK
- userspace should fill out_fences_ptr with the crtc_ids for which
it wants fences back.

v4: Create OUT_FENCE_PTR properties and remove old approach.

Signed-off-by: Gustavo Padovan 
---
drivers/gpu/drm/drm_atomic.c | 116 ++-
drivers/gpu/drm/drm_crtc.c   |   8 +++
include/drm/drm_crtc.h   |  19 +++
3 files changed, 119 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index b3284b2..07775fc 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -490,6 +490,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
);
state->color_mgmt_changed |= replaced;
return ret;
+   } else if (property == config->prop_out_fence_ptr) {
+   state->out_fence_ptr = u64_to_user_ptr(val);
} else if (crtc->funcs->atomic_set_property)
return crtc->funcs->atomic_set_property(crtc, state, property, 
val);
else
@@ -532,6 +534,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
*val = (state->ctm) ? state->ctm->base.id : 0;
else if (property == config->gamma_lut_property)
*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
+   else if (property == config->prop_out_fence_ptr)
+   *val = 0;
else if (crtc->funcs->atomic_get_property)
return crtc->funcs->atomic_get_property(crtc, state, property, 
val);
else
@@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
 */

static struct drm_pending_vblank_event *create_vblank_event(
-   struct drm_device *dev, struct drm_file *file_priv,
-   struct fence *fence, uint64_t user_data)
+   struct drm_device *dev, uint64_t user_data)
{
struct drm_pending_vblank_event *e = NULL;
-   int ret;

e = kzalloc(sizeof *e, GFP_KERNEL);
if (!e)
@@ -1488,17 +1490,6 @@ static struct drm_pending_vblank_event 
*create_vblank_event(
e->event.base.length = sizeof(e->event);
e->event.user_data = user_data;

-   if (file_priv) {
-   ret = drm_event_reserve_init(dev, file_priv, >base,
->event.base);
-   if (ret) {
-   kfree(e);
-   return NULL;
-   }
-   }
-
-   e->base.fence = fence;
-
return e;
}

@@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
}
EXPORT_SYMBOL(drm_atomic_clean_old_fb);

+static int crtc_setup_out_fence(struct drm_crtc *crtc,
+   struct drm_crtc_state *crtc_state,
+   struct drm_out_fence_state *fence_state)
+{
+   struct fence *fence;
+
+   fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
+   if (fence_state->fd < 0) {
+   return fence_state->fd;
+   }
+
+   fence_state->out_fence_ptr = crtc_state->out_fence_ptr;
+   crtc_state->out_fence_ptr = NULL;
+
+   if (put_user(fence_state->fd, fence_state->out_fence_ptr))
+   return -EFAULT;
+
+   fence = kzalloc(sizeof(*fence), GFP_KERNEL);
+   if (!fence)
+   return -ENOMEM;
+
+   fence_init(fence, _crtc_fence_ops, >fence_lock,
+  crtc->fence_context, ++crtc->fence_seqno);
+
+   fence_state->sync_file = sync_file_create(fence);
+   if(!fence_state->sync_file) {
+   fence_put(fence);
+   return -ENOMEM;
+   }
+
+   crtc_state->event->base.fence = fence_get(fence);
+   return 0;
+}
+
int drm_mode_atomic_ioctl(struct drm_device *dev,
  void *data, struct drm_file *file_priv)
{
@@ -1617,9 +1642,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
struct drm_plane *plane;
struct drm_crtc *crtc;
struct drm_crtc_state *crtc_state;
+   struct drm_out_fence_state *fence_state;
unsigned plane_mask;
int ret = 0;
-   unsigned int i, j;
+

Re: [PATCH v5 4/4] drm/fence: add out-fences support

2016-10-21 Thread Gustavo Padovan

2016-10-21 Daniel Vetter :

> On Thu, Oct 20, 2016 at 10:15:20PM +0300, Ville Syrjälä wrote:
> > On Thu, Oct 20, 2016 at 07:34:44PM +0300, Ville Syrjälä wrote:
> > > On Thu, Oct 20, 2016 at 01:55:38PM -0200, Gustavo Padovan wrote:
> > > > 2016-10-20 Ville Syrjälä :
> > > > 
> > > > > On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
> > > > > > From: Gustavo Padovan 
> > > > > > 
> > > > > > Support DRM out-fences by creating a sync_file with a fence for 
> > > > > > each CRTC
> > > > > > that sets the OUT_FENCE_PTR property.
> > > > > 
> > > > > I still maintain the out fence should also be per fb (well, per plane
> > > > > since we can't add it to fbs). Otherwise it's not going to be at all
> > > > > useful if you do fps>vrefresh and don't include the same set of planes
> > > > > in every atomic ioctl, eg. if you only ever include ones that are
> > > > > somehow dirty.
> > > > 
> > > > How would the kernel signal these dirty planes then? Right now we signal
> > > > at the vblank.
> > > 
> > > So if I think about it in terms of the previous fbs something like this
> > > comes to mind:
> > > 
> > >  starting point:
> > >  plane a, fb 0
> > >  plane b, fb 1
> > > 
> > >  ioctl:
> > >  plane a, fb 2, fence A
> > >  plane b, fb 3, fence B
> > > 
> > >  ioctl:
> > >  plane a, fb 4, fence C
> > >  -> fb 2 can be reused, so fence C should signal immediately?
> > > 
> > >  vblank:
> > >  -> fb 0,1 can be reused, so fence A,B signal?
> > > 
> > > It feels a bit wonky since the fence is effectively associated with the
> > > previous fb after the previous fb was already submitted to the kernel.
> > > One might assume fence A to be the one signalled early, but that would
> > > give the impression that fb 0 would be free for reuse when it's not.
> > 
> > OK, so after hashing this out on irc with Gustavo and Brian, we came
> > to the conclusion that the per-crtc out fence should be sufficient
> > after all.
> > 
> > The way it can work is that the first ioctl will return the fence,
> > doesn't really matter how many of the planes are involved in this
> > ioctl. Subsequent ioctls that manage to get in before the next
> > vblank will get back a -1 as the fence, even if the set if planes
> > involved is not the same as in the first ioctl. From the -1
> > userspace can tell what happened, and can then consult its own
> > records to see which still pending flips were overwritten, and
> > thus knows which buffers can be reused immediately.
> > 
> > If userspace plans on sending out the now free buffers to someone
> > else accompanied by a fence, it can just create an already signalled
> > fence to send instead of sending the fence it got back from the
> > atomic ioctl since that one is still associated with the original
> > scanout buffers.
> > 
> > The fence returned by the atomic ioctl will only signal after the
> > vblank, at which point userspace will obviously know that the
> > orginal scanout buffers are now also ready for reuse, and that
> > the last buffer submitted for each plane is now being actively
> > scanned out. So if userspace wants to send out some of the
> > original buffers to someone else, it can send along the fence
> > returned from the atomic ioctl.
> > 
> > So requires a bit more work from userspace perhaps. But obviously
> > it only has to do this if it plans on submitting multiple operations
> > per frame.
> > 
> > So a slightly expanded version of my previous example might look like:
> > starting point:
> >  plane a, fb 0
> >  plane b, fb 1
> > 
> > ioctl:
> >  crtc: fence A
> >  plane a, fb 2
> >  plane b, fb 3
> > 
> > ioctl:
> >  crtc: fence -1
> >  plane a, fb 4
> >  -> the previously submitted fb for plane a (fb 2) can be reused
> > 
> > ioctl:
> >  crtc: fence -1
> >  plane a, fb 5
> >  -> the previously submitted fb for plane a (fb 4) can be reused
> > 
> > vblank:
> >  -> fence A signals, fb 0,1 can be reused
> > fb 3 and 5 being scanned out now
> > 
> > 
> > We also had a quick side track w.r.t. variable refresh rate displays,
> > and I think the conclusion was that there the vblank may just happen
> > sooner or later. Nothing else should change. What's unclear is how
> > the refresh rate would be controlled. The two obvious options are
> > explicit control, and automagic based on the submit rate. But that's
> > a separate topic entirely.
> 
> Thanks a lot for doing this discussion and the detailed write-up. Imo we
> should bake this into the kerneldoc, as uabi documentation examples.
> Gustavo, can you pls include this? I'd put it into the kerneldoc for
> @out_fence, with inline struct comments we can be rather excessive in
> their lenght ;-)

Sure, I'll work on kernel doc for this.

Gustavo

Re: [PATCH v5 4/4] drm/fence: add out-fences support

2016-10-21 Thread Gustavo Padovan

2016-10-21 Daniel Vetter :

> On Thu, Oct 20, 2016 at 10:15:20PM +0300, Ville Syrjälä wrote:
> > On Thu, Oct 20, 2016 at 07:34:44PM +0300, Ville Syrjälä wrote:
> > > On Thu, Oct 20, 2016 at 01:55:38PM -0200, Gustavo Padovan wrote:
> > > > 2016-10-20 Ville Syrjälä :
> > > > 
> > > > > On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
> > > > > > From: Gustavo Padovan 
> > > > > > 
> > > > > > Support DRM out-fences by creating a sync_file with a fence for 
> > > > > > each CRTC
> > > > > > that sets the OUT_FENCE_PTR property.
> > > > > 
> > > > > I still maintain the out fence should also be per fb (well, per plane
> > > > > since we can't add it to fbs). Otherwise it's not going to be at all
> > > > > useful if you do fps>vrefresh and don't include the same set of planes
> > > > > in every atomic ioctl, eg. if you only ever include ones that are
> > > > > somehow dirty.
> > > > 
> > > > How would the kernel signal these dirty planes then? Right now we signal
> > > > at the vblank.
> > > 
> > > So if I think about it in terms of the previous fbs something like this
> > > comes to mind:
> > > 
> > >  starting point:
> > >  plane a, fb 0
> > >  plane b, fb 1
> > > 
> > >  ioctl:
> > >  plane a, fb 2, fence A
> > >  plane b, fb 3, fence B
> > > 
> > >  ioctl:
> > >  plane a, fb 4, fence C
> > >  -> fb 2 can be reused, so fence C should signal immediately?
> > > 
> > >  vblank:
> > >  -> fb 0,1 can be reused, so fence A,B signal?
> > > 
> > > It feels a bit wonky since the fence is effectively associated with the
> > > previous fb after the previous fb was already submitted to the kernel.
> > > One might assume fence A to be the one signalled early, but that would
> > > give the impression that fb 0 would be free for reuse when it's not.
> > 
> > OK, so after hashing this out on irc with Gustavo and Brian, we came
> > to the conclusion that the per-crtc out fence should be sufficient
> > after all.
> > 
> > The way it can work is that the first ioctl will return the fence,
> > doesn't really matter how many of the planes are involved in this
> > ioctl. Subsequent ioctls that manage to get in before the next
> > vblank will get back a -1 as the fence, even if the set if planes
> > involved is not the same as in the first ioctl. From the -1
> > userspace can tell what happened, and can then consult its own
> > records to see which still pending flips were overwritten, and
> > thus knows which buffers can be reused immediately.
> > 
> > If userspace plans on sending out the now free buffers to someone
> > else accompanied by a fence, it can just create an already signalled
> > fence to send instead of sending the fence it got back from the
> > atomic ioctl since that one is still associated with the original
> > scanout buffers.
> > 
> > The fence returned by the atomic ioctl will only signal after the
> > vblank, at which point userspace will obviously know that the
> > orginal scanout buffers are now also ready for reuse, and that
> > the last buffer submitted for each plane is now being actively
> > scanned out. So if userspace wants to send out some of the
> > original buffers to someone else, it can send along the fence
> > returned from the atomic ioctl.
> > 
> > So requires a bit more work from userspace perhaps. But obviously
> > it only has to do this if it plans on submitting multiple operations
> > per frame.
> > 
> > So a slightly expanded version of my previous example might look like:
> > starting point:
> >  plane a, fb 0
> >  plane b, fb 1
> > 
> > ioctl:
> >  crtc: fence A
> >  plane a, fb 2
> >  plane b, fb 3
> > 
> > ioctl:
> >  crtc: fence -1
> >  plane a, fb 4
> >  -> the previously submitted fb for plane a (fb 2) can be reused
> > 
> > ioctl:
> >  crtc: fence -1
> >  plane a, fb 5
> >  -> the previously submitted fb for plane a (fb 4) can be reused
> > 
> > vblank:
> >  -> fence A signals, fb 0,1 can be reused
> > fb 3 and 5 being scanned out now
> > 
> > 
> > We also had a quick side track w.r.t. variable refresh rate displays,
> > and I think the conclusion was that there the vblank may just happen
> > sooner or later. Nothing else should change. What's unclear is how
> > the refresh rate would be controlled. The two obvious options are
> > explicit control, and automagic based on the submit rate. But that's
> > a separate topic entirely.
> 
> Thanks a lot for doing this discussion and the detailed write-up. Imo we
> should bake this into the kerneldoc, as uabi documentation examples.
> Gustavo, can you pls include this? I'd put it into the kerneldoc for
> @out_fence, with inline struct comments we can be rather excessive in
> their lenght ;-)

Sure, I'll work on kernel doc for this.

Gustavo

Re: [PATCH v5 4/4] drm/fence: add out-fences support

2016-10-21 Thread Daniel Vetter

On Fri, Oct 21, 2016 at 11:55:52AM +0100, Brian Starkey wrote:
> On Thu, Oct 20, 2016 at 06:30:17PM -0200, Gustavo Padovan wrote:
> > 2016-10-20 Brian Starkey :
> > > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > > > index 657a33a..b898604 100644
> > > > --- a/include/drm/drm_crtc.h
> > > > +++ b/include/drm/drm_crtc.h
> > > > @@ -165,6 +165,8 @@ struct drm_crtc_state {
> > > > struct drm_property_blob *ctm;
> > > > struct drm_property_blob *gamma_lut;
> > > >
> > > > +   u64 __user *out_fence_ptr;
> > > > +
> > > 
> > > I'm somewhat not convinced about stashing a __user pointer in the
> > > CRTC atomic state. I know it gets cleared before the driver sees it,
> > > but if anything I think that hints that this isn't the right place to
> > > store it.
> > > 
> > > I've been trying to think of other ways to get from a property to
> > > something drm_mode_atomic_ioctl can use - maybe we can store some
> > > stuff in drm_atomic_state which only lives for the length of the ioctl
> > > call and put this pointer in there.
> > 
> > The drm_atomic_state is still visible by the drivers. Between there and
> > crtc_state, I would keep in the crtc_state for now.
> > 
> 
> Mm, yeah I suppose they could get to it if they went looking for it
> in ->atomic_set_property or something, but I was thinking of freeing
> it before the drm_atomic_commit.
> 
> Anyway, this way is certainly simpler, and setting it to NULL should
> be enough to limit the damage a driver can do :-)

+1 on moving this out of drm_crtc_state. drm_atomic_state already has
per-crtc structs, so easy to extend them with this. And yes drivers can
still see it, but mostly they're not supposed to touch drm_atomic_state
internals - the book-keeping is all done by the core.

The per-object state structs otoh are meant to be massively mangled by
drivers.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v5 4/4] drm/fence: add out-fences support

2016-10-21 Thread Daniel Vetter

On Fri, Oct 21, 2016 at 11:55:52AM +0100, Brian Starkey wrote:
> On Thu, Oct 20, 2016 at 06:30:17PM -0200, Gustavo Padovan wrote:
> > 2016-10-20 Brian Starkey :
> > > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > > > index 657a33a..b898604 100644
> > > > --- a/include/drm/drm_crtc.h
> > > > +++ b/include/drm/drm_crtc.h
> > > > @@ -165,6 +165,8 @@ struct drm_crtc_state {
> > > > struct drm_property_blob *ctm;
> > > > struct drm_property_blob *gamma_lut;
> > > >
> > > > +   u64 __user *out_fence_ptr;
> > > > +
> > > 
> > > I'm somewhat not convinced about stashing a __user pointer in the
> > > CRTC atomic state. I know it gets cleared before the driver sees it,
> > > but if anything I think that hints that this isn't the right place to
> > > store it.
> > > 
> > > I've been trying to think of other ways to get from a property to
> > > something drm_mode_atomic_ioctl can use - maybe we can store some
> > > stuff in drm_atomic_state which only lives for the length of the ioctl
> > > call and put this pointer in there.
> > 
> > The drm_atomic_state is still visible by the drivers. Between there and
> > crtc_state, I would keep in the crtc_state for now.
> > 
> 
> Mm, yeah I suppose they could get to it if they went looking for it
> in ->atomic_set_property or something, but I was thinking of freeing
> it before the drm_atomic_commit.
> 
> Anyway, this way is certainly simpler, and setting it to NULL should
> be enough to limit the damage a driver can do :-)

+1 on moving this out of drm_crtc_state. drm_atomic_state already has
per-crtc structs, so easy to extend them with this. And yes drivers can
still see it, but mostly they're not supposed to touch drm_atomic_state
internals - the book-keeping is all done by the core.

The per-object state structs otoh are meant to be massively mangled by
drivers.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v5 4/4] drm/fence: add out-fences support

2016-10-21 Thread Daniel Vetter

On Thu, Oct 20, 2016 at 10:15:20PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 20, 2016 at 07:34:44PM +0300, Ville Syrjälä wrote:
> > On Thu, Oct 20, 2016 at 01:55:38PM -0200, Gustavo Padovan wrote:
> > > 2016-10-20 Ville Syrjälä :
> > > 
> > > > On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
> > > > > From: Gustavo Padovan 
> > > > > 
> > > > > Support DRM out-fences by creating a sync_file with a fence for each 
> > > > > CRTC
> > > > > that sets the OUT_FENCE_PTR property.
> > > > 
> > > > I still maintain the out fence should also be per fb (well, per plane
> > > > since we can't add it to fbs). Otherwise it's not going to be at all
> > > > useful if you do fps>vrefresh and don't include the same set of planes
> > > > in every atomic ioctl, eg. if you only ever include ones that are
> > > > somehow dirty.
> > > 
> > > How would the kernel signal these dirty planes then? Right now we signal
> > > at the vblank.
> > 
> > So if I think about it in terms of the previous fbs something like this
> > comes to mind:
> > 
> >  starting point:
> >  plane a, fb 0
> >  plane b, fb 1
> > 
> >  ioctl:
> >  plane a, fb 2, fence A
> >  plane b, fb 3, fence B
> > 
> >  ioctl:
> >  plane a, fb 4, fence C
> >  -> fb 2 can be reused, so fence C should signal immediately?
> > 
> >  vblank:
> >  -> fb 0,1 can be reused, so fence A,B signal?
> > 
> > It feels a bit wonky since the fence is effectively associated with the
> > previous fb after the previous fb was already submitted to the kernel.
> > One might assume fence A to be the one signalled early, but that would
> > give the impression that fb 0 would be free for reuse when it's not.
> 
> OK, so after hashing this out on irc with Gustavo and Brian, we came
> to the conclusion that the per-crtc out fence should be sufficient
> after all.
> 
> The way it can work is that the first ioctl will return the fence,
> doesn't really matter how many of the planes are involved in this
> ioctl. Subsequent ioctls that manage to get in before the next
> vblank will get back a -1 as the fence, even if the set if planes
> involved is not the same as in the first ioctl. From the -1
> userspace can tell what happened, and can then consult its own
> records to see which still pending flips were overwritten, and
> thus knows which buffers can be reused immediately.
> 
> If userspace plans on sending out the now free buffers to someone
> else accompanied by a fence, it can just create an already signalled
> fence to send instead of sending the fence it got back from the
> atomic ioctl since that one is still associated with the original
> scanout buffers.
> 
> The fence returned by the atomic ioctl will only signal after the
> vblank, at which point userspace will obviously know that the
> orginal scanout buffers are now also ready for reuse, and that
> the last buffer submitted for each plane is now being actively
> scanned out. So if userspace wants to send out some of the
> original buffers to someone else, it can send along the fence
> returned from the atomic ioctl.
> 
> So requires a bit more work from userspace perhaps. But obviously
> it only has to do this if it plans on submitting multiple operations
> per frame.
> 
> So a slightly expanded version of my previous example might look like:
> starting point:
>  plane a, fb 0
>  plane b, fb 1
> 
> ioctl:
>  crtc: fence A
>  plane a, fb 2
>  plane b, fb 3
> 
> ioctl:
>  crtc: fence -1
>  plane a, fb 4
>  -> the previously submitted fb for plane a (fb 2) can be reused
> 
> ioctl:
>  crtc: fence -1
>  plane a, fb 5
>  -> the previously submitted fb for plane a (fb 4) can be reused
> 
> vblank:
>  -> fence A signals, fb 0,1 can be reused
> fb 3 and 5 being scanned out now
> 
> 
> We also had a quick side track w.r.t. variable refresh rate displays,
> and I think the conclusion was that there the vblank may just happen
> sooner or later. Nothing else should change. What's unclear is how
> the refresh rate would be controlled. The two obvious options are
> explicit control, and automagic based on the submit rate. But that's
> a separate topic entirely.

Thanks a lot for doing this discussion and the detailed write-up. Imo we
should bake this into the kerneldoc, as uabi documentation examples.
Gustavo, can you pls include this? I'd put it into the kerneldoc for
@out_fence, with inline struct comments we can be rather excessive in
their lenght ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v5 4/4] drm/fence: add out-fences support

2016-10-21 Thread Daniel Vetter

On Thu, Oct 20, 2016 at 10:15:20PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 20, 2016 at 07:34:44PM +0300, Ville Syrjälä wrote:
> > On Thu, Oct 20, 2016 at 01:55:38PM -0200, Gustavo Padovan wrote:
> > > 2016-10-20 Ville Syrjälä :
> > > 
> > > > On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
> > > > > From: Gustavo Padovan 
> > > > > 
> > > > > Support DRM out-fences by creating a sync_file with a fence for each 
> > > > > CRTC
> > > > > that sets the OUT_FENCE_PTR property.
> > > > 
> > > > I still maintain the out fence should also be per fb (well, per plane
> > > > since we can't add it to fbs). Otherwise it's not going to be at all
> > > > useful if you do fps>vrefresh and don't include the same set of planes
> > > > in every atomic ioctl, eg. if you only ever include ones that are
> > > > somehow dirty.
> > > 
> > > How would the kernel signal these dirty planes then? Right now we signal
> > > at the vblank.
> > 
> > So if I think about it in terms of the previous fbs something like this
> > comes to mind:
> > 
> >  starting point:
> >  plane a, fb 0
> >  plane b, fb 1
> > 
> >  ioctl:
> >  plane a, fb 2, fence A
> >  plane b, fb 3, fence B
> > 
> >  ioctl:
> >  plane a, fb 4, fence C
> >  -> fb 2 can be reused, so fence C should signal immediately?
> > 
> >  vblank:
> >  -> fb 0,1 can be reused, so fence A,B signal?
> > 
> > It feels a bit wonky since the fence is effectively associated with the
> > previous fb after the previous fb was already submitted to the kernel.
> > One might assume fence A to be the one signalled early, but that would
> > give the impression that fb 0 would be free for reuse when it's not.
> 
> OK, so after hashing this out on irc with Gustavo and Brian, we came
> to the conclusion that the per-crtc out fence should be sufficient
> after all.
> 
> The way it can work is that the first ioctl will return the fence,
> doesn't really matter how many of the planes are involved in this
> ioctl. Subsequent ioctls that manage to get in before the next
> vblank will get back a -1 as the fence, even if the set if planes
> involved is not the same as in the first ioctl. From the -1
> userspace can tell what happened, and can then consult its own
> records to see which still pending flips were overwritten, and
> thus knows which buffers can be reused immediately.
> 
> If userspace plans on sending out the now free buffers to someone
> else accompanied by a fence, it can just create an already signalled
> fence to send instead of sending the fence it got back from the
> atomic ioctl since that one is still associated with the original
> scanout buffers.
> 
> The fence returned by the atomic ioctl will only signal after the
> vblank, at which point userspace will obviously know that the
> orginal scanout buffers are now also ready for reuse, and that
> the last buffer submitted for each plane is now being actively
> scanned out. So if userspace wants to send out some of the
> original buffers to someone else, it can send along the fence
> returned from the atomic ioctl.
> 
> So requires a bit more work from userspace perhaps. But obviously
> it only has to do this if it plans on submitting multiple operations
> per frame.
> 
> So a slightly expanded version of my previous example might look like:
> starting point:
>  plane a, fb 0
>  plane b, fb 1
> 
> ioctl:
>  crtc: fence A
>  plane a, fb 2
>  plane b, fb 3
> 
> ioctl:
>  crtc: fence -1
>  plane a, fb 4
>  -> the previously submitted fb for plane a (fb 2) can be reused
> 
> ioctl:
>  crtc: fence -1
>  plane a, fb 5
>  -> the previously submitted fb for plane a (fb 4) can be reused
> 
> vblank:
>  -> fence A signals, fb 0,1 can be reused
> fb 3 and 5 being scanned out now
> 
> 
> We also had a quick side track w.r.t. variable refresh rate displays,
> and I think the conclusion was that there the vblank may just happen
> sooner or later. Nothing else should change. What's unclear is how
> the refresh rate would be controlled. The two obvious options are
> explicit control, and automagic based on the submit rate. But that's
> a separate topic entirely.

Thanks a lot for doing this discussion and the detailed write-up. Imo we
should bake this into the kerneldoc, as uabi documentation examples.
Gustavo, can you pls include this? I'd put it into the kerneldoc for
@out_fence, with inline struct comments we can be rather excessive in
their lenght ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v5 4/4] drm/fence: add out-fences support

2016-10-21 Thread Brian Starkey


Hi Gustavo,

On Thu, Oct 20, 2016 at 06:30:17PM -0200, Gustavo Padovan wrote:

Hi Brian,

2016-10-20 Brian Starkey :


Hi Gustavo,

I notice your branch has the sync_file refcount change in, but this
doesn't seem to take account for that. Will you be dropping that
change to match the semantics of fence_array?


I will drop the fence_get() in the out-fence patch because we already
hold the ref when we create the fence. The sync_file refcount patch will
remain.



OK, I'll work on that basis, thanks.



Couple more comments below.

On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
> From: Gustavo Padovan 
>
> Support DRM out-fences by creating a sync_file with a fence for each CRTC
> that sets the OUT_FENCE_PTR property.
>
> We use the out_fence pointer received in the OUT_FENCE_PTR prop to send
> the sync_file fd back to userspace.
>
> The sync_file and fd are allocated/created before commit, but the
> fd_install operation only happens after we know that commit succeed.
>
> In case of error userspace should receive a fd number of -1.
>
> v2: Comment by Rob Clark:
>- Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here.
>
>Comment by Daniel Vetter:
>- Add clean up code for out_fences
>
> v3: Comments by Daniel Vetter:
>- create DRM_MODE_ATOMIC_EVENT_MASK
>- userspace should fill out_fences_ptr with the crtc_ids for which
>it wants fences back.
>
> v4: Create OUT_FENCE_PTR properties and remove old approach.
>
> Signed-off-by: Gustavo Padovan 
> ---
> drivers/gpu/drm/drm_atomic.c | 116 ++-
> drivers/gpu/drm/drm_crtc.c   |   8 +++
> include/drm/drm_crtc.h   |  19 +++
> 3 files changed, 119 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index b3284b2..07775fc 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -490,6 +490,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>);
>state->color_mgmt_changed |= replaced;
>return ret;
> +  } else if (property == config->prop_out_fence_ptr) {
> +  state->out_fence_ptr = u64_to_user_ptr(val);
>} else if (crtc->funcs->atomic_set_property)
>return crtc->funcs->atomic_set_property(crtc, state, property, 
val);
>else
> @@ -532,6 +534,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>*val = (state->ctm) ? state->ctm->base.id : 0;
>else if (property == config->gamma_lut_property)
>*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
> +  else if (property == config->prop_out_fence_ptr)
> +  *val = 0;
>else if (crtc->funcs->atomic_get_property)
>return crtc->funcs->atomic_get_property(crtc, state, property, 
val);
>else
> @@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
>  */
>
> static struct drm_pending_vblank_event *create_vblank_event(
> -  struct drm_device *dev, struct drm_file *file_priv,
> -  struct fence *fence, uint64_t user_data)
> +  struct drm_device *dev, uint64_t user_data)
> {
>struct drm_pending_vblank_event *e = NULL;
> -  int ret;
>
>e = kzalloc(sizeof *e, GFP_KERNEL);
>if (!e)
> @@ -1488,17 +1490,6 @@ static struct drm_pending_vblank_event 
*create_vblank_event(
>e->event.base.length = sizeof(e->event);
>e->event.user_data = user_data;
>
> -  if (file_priv) {
> -  ret = drm_event_reserve_init(dev, file_priv, >base,
> -   >event.base);
> -  if (ret) {
> -  kfree(e);
> -  return NULL;
> -  }
> -  }
> -
> -  e->base.fence = fence;
> -
>return e;
> }
>
> @@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
> }
> EXPORT_SYMBOL(drm_atomic_clean_old_fb);
>
> +static int crtc_setup_out_fence(struct drm_crtc *crtc,
> +  struct drm_crtc_state *crtc_state,
> +  struct drm_out_fence_state *fence_state)
> +{
> +  struct fence *fence;
> +
> +  fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
> +  if (fence_state->fd < 0) {
> +  return fence_state->fd;
> +  }
> +
> +  fence_state->out_fence_ptr = crtc_state->out_fence_ptr;
> +  crtc_state->out_fence_ptr = NULL;
> +
> +  if (put_user(fence_state->fd, fence_state->out_fence_ptr))
> +  return -EFAULT;
> +
> +  fence = kzalloc(sizeof(*fence), GFP_KERNEL);
> +  if (!fence)
> +  return -ENOMEM;
> +
> +  fence_init(fence, _crtc_fence_ops, >fence_lock,
> + crtc->fence_context, ++crtc->fence_seqno);
> +
> +  fence_state->sync_file = sync_file_create(fence);
> +  if(!fence_state->sync_file) {
> +  fence_put(fence);
> +  return -ENOMEM;
> +  }
> +
> +  crtc_state->event->base.fence = fence_get(fence);
> +  return

Re: [PATCH v5 4/4] drm/fence: add out-fences support

2016-10-21 Thread Brian Starkey


Hi Gustavo,

On Thu, Oct 20, 2016 at 06:30:17PM -0200, Gustavo Padovan wrote:

Hi Brian,

2016-10-20 Brian Starkey :


Hi Gustavo,

I notice your branch has the sync_file refcount change in, but this
doesn't seem to take account for that. Will you be dropping that
change to match the semantics of fence_array?


I will drop the fence_get() in the out-fence patch because we already
hold the ref when we create the fence. The sync_file refcount patch will
remain.



OK, I'll work on that basis, thanks.



Couple more comments below.

On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
> From: Gustavo Padovan 
>
> Support DRM out-fences by creating a sync_file with a fence for each CRTC
> that sets the OUT_FENCE_PTR property.
>
> We use the out_fence pointer received in the OUT_FENCE_PTR prop to send
> the sync_file fd back to userspace.
>
> The sync_file and fd are allocated/created before commit, but the
> fd_install operation only happens after we know that commit succeed.
>
> In case of error userspace should receive a fd number of -1.
>
> v2: Comment by Rob Clark:
>- Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here.
>
>Comment by Daniel Vetter:
>- Add clean up code for out_fences
>
> v3: Comments by Daniel Vetter:
>- create DRM_MODE_ATOMIC_EVENT_MASK
>- userspace should fill out_fences_ptr with the crtc_ids for which
>it wants fences back.
>
> v4: Create OUT_FENCE_PTR properties and remove old approach.
>
> Signed-off-by: Gustavo Padovan 
> ---
> drivers/gpu/drm/drm_atomic.c | 116 ++-
> drivers/gpu/drm/drm_crtc.c   |   8 +++
> include/drm/drm_crtc.h   |  19 +++
> 3 files changed, 119 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index b3284b2..07775fc 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -490,6 +490,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>);
>state->color_mgmt_changed |= replaced;
>return ret;
> +  } else if (property == config->prop_out_fence_ptr) {
> +  state->out_fence_ptr = u64_to_user_ptr(val);
>} else if (crtc->funcs->atomic_set_property)
>return crtc->funcs->atomic_set_property(crtc, state, property, 
val);
>else
> @@ -532,6 +534,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>*val = (state->ctm) ? state->ctm->base.id : 0;
>else if (property == config->gamma_lut_property)
>*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
> +  else if (property == config->prop_out_fence_ptr)
> +  *val = 0;
>else if (crtc->funcs->atomic_get_property)
>return crtc->funcs->atomic_get_property(crtc, state, property, 
val);
>else
> @@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
>  */
>
> static struct drm_pending_vblank_event *create_vblank_event(
> -  struct drm_device *dev, struct drm_file *file_priv,
> -  struct fence *fence, uint64_t user_data)
> +  struct drm_device *dev, uint64_t user_data)
> {
>struct drm_pending_vblank_event *e = NULL;
> -  int ret;
>
>e = kzalloc(sizeof *e, GFP_KERNEL);
>if (!e)
> @@ -1488,17 +1490,6 @@ static struct drm_pending_vblank_event 
*create_vblank_event(
>e->event.base.length = sizeof(e->event);
>e->event.user_data = user_data;
>
> -  if (file_priv) {
> -  ret = drm_event_reserve_init(dev, file_priv, >base,
> -   >event.base);
> -  if (ret) {
> -  kfree(e);
> -  return NULL;
> -  }
> -  }
> -
> -  e->base.fence = fence;
> -
>return e;
> }
>
> @@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
> }
> EXPORT_SYMBOL(drm_atomic_clean_old_fb);
>
> +static int crtc_setup_out_fence(struct drm_crtc *crtc,
> +  struct drm_crtc_state *crtc_state,
> +  struct drm_out_fence_state *fence_state)
> +{
> +  struct fence *fence;
> +
> +  fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
> +  if (fence_state->fd < 0) {
> +  return fence_state->fd;
> +  }
> +
> +  fence_state->out_fence_ptr = crtc_state->out_fence_ptr;
> +  crtc_state->out_fence_ptr = NULL;
> +
> +  if (put_user(fence_state->fd, fence_state->out_fence_ptr))
> +  return -EFAULT;
> +
> +  fence = kzalloc(sizeof(*fence), GFP_KERNEL);
> +  if (!fence)
> +  return -ENOMEM;
> +
> +  fence_init(fence, _crtc_fence_ops, >fence_lock,
> + crtc->fence_context, ++crtc->fence_seqno);
> +
> +  fence_state->sync_file = sync_file_create(fence);
> +  if(!fence_state->sync_file) {
> +  fence_put(fence);
> +  return -ENOMEM;
> +  }
> +
> +  crtc_state->event->base.fence = fence_get(fence);
> +  return 0;
> +}
> +
> int drm_mode_atomic_ioctl(struct drm_device *dev,
>

Re: [PATCH v5 4/4] drm/fence: add out-fences support

2016-10-20 Thread Gustavo Padovan

Hi Brian,

2016-10-20 Brian Starkey :

> Hi Gustavo,
> 
> I notice your branch has the sync_file refcount change in, but this
> doesn't seem to take account for that. Will you be dropping that
> change to match the semantics of fence_array?

I will drop the fence_get() in the out-fence patch because we already
hold the ref when we create the fence. The sync_file refcount patch will
remain.

> 
> Couple more comments below.
> 
> On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
> > From: Gustavo Padovan 
> > 
> > Support DRM out-fences by creating a sync_file with a fence for each CRTC
> > that sets the OUT_FENCE_PTR property.
> > 
> > We use the out_fence pointer received in the OUT_FENCE_PTR prop to send
> > the sync_file fd back to userspace.
> > 
> > The sync_file and fd are allocated/created before commit, but the
> > fd_install operation only happens after we know that commit succeed.
> > 
> > In case of error userspace should receive a fd number of -1.
> > 
> > v2: Comment by Rob Clark:
> > - Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here.
> > 
> >Comment by Daniel Vetter:
> > - Add clean up code for out_fences
> > 
> > v3: Comments by Daniel Vetter:
> > - create DRM_MODE_ATOMIC_EVENT_MASK
> > - userspace should fill out_fences_ptr with the crtc_ids for which
> > it wants fences back.
> > 
> > v4: Create OUT_FENCE_PTR properties and remove old approach.
> > 
> > Signed-off-by: Gustavo Padovan 
> > ---
> > drivers/gpu/drm/drm_atomic.c | 116 
> > ++-
> > drivers/gpu/drm/drm_crtc.c   |   8 +++
> > include/drm/drm_crtc.h   |  19 +++
> > 3 files changed, 119 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index b3284b2..07775fc 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -490,6 +490,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> > );
> > state->color_mgmt_changed |= replaced;
> > return ret;
> > +   } else if (property == config->prop_out_fence_ptr) {
> > +   state->out_fence_ptr = u64_to_user_ptr(val);
> > } else if (crtc->funcs->atomic_set_property)
> > return crtc->funcs->atomic_set_property(crtc, state, property, 
> > val);
> > else
> > @@ -532,6 +534,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
> > *val = (state->ctm) ? state->ctm->base.id : 0;
> > else if (property == config->gamma_lut_property)
> > *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
> > +   else if (property == config->prop_out_fence_ptr)
> > +   *val = 0;
> > else if (crtc->funcs->atomic_get_property)
> > return crtc->funcs->atomic_get_property(crtc, state, property, 
> > val);
> > else
> > @@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
> >  */
> > 
> > static struct drm_pending_vblank_event *create_vblank_event(
> > -   struct drm_device *dev, struct drm_file *file_priv,
> > -   struct fence *fence, uint64_t user_data)
> > +   struct drm_device *dev, uint64_t user_data)
> > {
> > struct drm_pending_vblank_event *e = NULL;
> > -   int ret;
> > 
> > e = kzalloc(sizeof *e, GFP_KERNEL);
> > if (!e)
> > @@ -1488,17 +1490,6 @@ static struct drm_pending_vblank_event 
> > *create_vblank_event(
> > e->event.base.length = sizeof(e->event);
> > e->event.user_data = user_data;
> > 
> > -   if (file_priv) {
> > -   ret = drm_event_reserve_init(dev, file_priv, >base,
> > ->event.base);
> > -   if (ret) {
> > -   kfree(e);
> > -   return NULL;
> > -   }
> > -   }
> > -
> > -   e->base.fence = fence;
> > -
> > return e;
> > }
> > 
> > @@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
> > }
> > EXPORT_SYMBOL(drm_atomic_clean_old_fb);
> > 
> > +static int crtc_setup_out_fence(struct drm_crtc *crtc,
> > +   struct drm_crtc_state *crtc_state,
> > +   struct drm_out_fence_state *fence_state)
> > +{
> > +   struct fence *fence;
> > +
> > +   fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
> > +   if (fence_state->fd < 0) {
> > +   return fence_state->fd;
> > +   }
> > +
> > +   fence_state->out_fence_ptr = crtc_state->out_fence_ptr;
> > +   crtc_state->out_fence_ptr = NULL;
> > +
> > +   if (put_user(fence_state->fd, fence_state->out_fence_ptr))
> > +   return -EFAULT;
> > +
> > +   fence = kzalloc(sizeof(*fence), GFP_KERNEL);
> > +   if (!fence)
> > +   return -ENOMEM;
> > +
> > +   fence_init(fence, _crtc_fence_ops, >fence_lock,
> > +  crtc->fence_context, ++crtc->fence_seqno);
> > +
> > +

Re: [PATCH v5 4/4] drm/fence: add out-fences support

2016-10-20 Thread Gustavo Padovan

Hi Brian,

2016-10-20 Brian Starkey :

> Hi Gustavo,
> 
> I notice your branch has the sync_file refcount change in, but this
> doesn't seem to take account for that. Will you be dropping that
> change to match the semantics of fence_array?

I will drop the fence_get() in the out-fence patch because we already
hold the ref when we create the fence. The sync_file refcount patch will
remain.

> 
> Couple more comments below.
> 
> On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
> > From: Gustavo Padovan 
> > 
> > Support DRM out-fences by creating a sync_file with a fence for each CRTC
> > that sets the OUT_FENCE_PTR property.
> > 
> > We use the out_fence pointer received in the OUT_FENCE_PTR prop to send
> > the sync_file fd back to userspace.
> > 
> > The sync_file and fd are allocated/created before commit, but the
> > fd_install operation only happens after we know that commit succeed.
> > 
> > In case of error userspace should receive a fd number of -1.
> > 
> > v2: Comment by Rob Clark:
> > - Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here.
> > 
> >Comment by Daniel Vetter:
> > - Add clean up code for out_fences
> > 
> > v3: Comments by Daniel Vetter:
> > - create DRM_MODE_ATOMIC_EVENT_MASK
> > - userspace should fill out_fences_ptr with the crtc_ids for which
> > it wants fences back.
> > 
> > v4: Create OUT_FENCE_PTR properties and remove old approach.
> > 
> > Signed-off-by: Gustavo Padovan 
> > ---
> > drivers/gpu/drm/drm_atomic.c | 116 
> > ++-
> > drivers/gpu/drm/drm_crtc.c   |   8 +++
> > include/drm/drm_crtc.h   |  19 +++
> > 3 files changed, 119 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index b3284b2..07775fc 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -490,6 +490,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> > );
> > state->color_mgmt_changed |= replaced;
> > return ret;
> > +   } else if (property == config->prop_out_fence_ptr) {
> > +   state->out_fence_ptr = u64_to_user_ptr(val);
> > } else if (crtc->funcs->atomic_set_property)
> > return crtc->funcs->atomic_set_property(crtc, state, property, 
> > val);
> > else
> > @@ -532,6 +534,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
> > *val = (state->ctm) ? state->ctm->base.id : 0;
> > else if (property == config->gamma_lut_property)
> > *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
> > +   else if (property == config->prop_out_fence_ptr)
> > +   *val = 0;
> > else if (crtc->funcs->atomic_get_property)
> > return crtc->funcs->atomic_get_property(crtc, state, property, 
> > val);
> > else
> > @@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
> >  */
> > 
> > static struct drm_pending_vblank_event *create_vblank_event(
> > -   struct drm_device *dev, struct drm_file *file_priv,
> > -   struct fence *fence, uint64_t user_data)
> > +   struct drm_device *dev, uint64_t user_data)
> > {
> > struct drm_pending_vblank_event *e = NULL;
> > -   int ret;
> > 
> > e = kzalloc(sizeof *e, GFP_KERNEL);
> > if (!e)
> > @@ -1488,17 +1490,6 @@ static struct drm_pending_vblank_event 
> > *create_vblank_event(
> > e->event.base.length = sizeof(e->event);
> > e->event.user_data = user_data;
> > 
> > -   if (file_priv) {
> > -   ret = drm_event_reserve_init(dev, file_priv, >base,
> > ->event.base);
> > -   if (ret) {
> > -   kfree(e);
> > -   return NULL;
> > -   }
> > -   }
> > -
> > -   e->base.fence = fence;
> > -
> > return e;
> > }
> > 
> > @@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
> > }
> > EXPORT_SYMBOL(drm_atomic_clean_old_fb);
> > 
> > +static int crtc_setup_out_fence(struct drm_crtc *crtc,
> > +   struct drm_crtc_state *crtc_state,
> > +   struct drm_out_fence_state *fence_state)
> > +{
> > +   struct fence *fence;
> > +
> > +   fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
> > +   if (fence_state->fd < 0) {
> > +   return fence_state->fd;
> > +   }
> > +
> > +   fence_state->out_fence_ptr = crtc_state->out_fence_ptr;
> > +   crtc_state->out_fence_ptr = NULL;
> > +
> > +   if (put_user(fence_state->fd, fence_state->out_fence_ptr))
> > +   return -EFAULT;
> > +
> > +   fence = kzalloc(sizeof(*fence), GFP_KERNEL);
> > +   if (!fence)
> > +   return -ENOMEM;
> > +
> > +   fence_init(fence, _crtc_fence_ops, >fence_lock,
> > +  crtc->fence_context, ++crtc->fence_seqno);
> > +
> > +   fence_state->sync_file = sync_file_create(fence);
> > +   if(!fence_state->sync_file) {
> > +

Re: [PATCH v5 4/4] drm/fence: add out-fences support

2016-10-20 Thread Ville Syrjälä

On Thu, Oct 20, 2016 at 07:34:44PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 20, 2016 at 01:55:38PM -0200, Gustavo Padovan wrote:
> > 2016-10-20 Ville Syrjälä :
> > 
> > > On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
> > > > From: Gustavo Padovan 
> > > > 
> > > > Support DRM out-fences by creating a sync_file with a fence for each 
> > > > CRTC
> > > > that sets the OUT_FENCE_PTR property.
> > > 
> > > I still maintain the out fence should also be per fb (well, per plane
> > > since we can't add it to fbs). Otherwise it's not going to be at all
> > > useful if you do fps>vrefresh and don't include the same set of planes
> > > in every atomic ioctl, eg. if you only ever include ones that are
> > > somehow dirty.
> > 
> > How would the kernel signal these dirty planes then? Right now we signal
> > at the vblank.
> 
> So if I think about it in terms of the previous fbs something like this
> comes to mind:
> 
>  starting point:
>  plane a, fb 0
>  plane b, fb 1
> 
>  ioctl:
>  plane a, fb 2, fence A
>  plane b, fb 3, fence B
> 
>  ioctl:
>  plane a, fb 4, fence C
>  -> fb 2 can be reused, so fence C should signal immediately?
> 
>  vblank:
>  -> fb 0,1 can be reused, so fence A,B signal?
> 
> It feels a bit wonky since the fence is effectively associated with the
> previous fb after the previous fb was already submitted to the kernel.
> One might assume fence A to be the one signalled early, but that would
> give the impression that fb 0 would be free for reuse when it's not.

OK, so after hashing this out on irc with Gustavo and Brian, we came
to the conclusion that the per-crtc out fence should be sufficient
after all.

The way it can work is that the first ioctl will return the fence,
doesn't really matter how many of the planes are involved in this
ioctl. Subsequent ioctls that manage to get in before the next
vblank will get back a -1 as the fence, even if the set if planes
involved is not the same as in the first ioctl. From the -1
userspace can tell what happened, and can then consult its own
records to see which still pending flips were overwritten, and
thus knows which buffers can be reused immediately.

If userspace plans on sending out the now free buffers to someone
else accompanied by a fence, it can just create an already signalled
fence to send instead of sending the fence it got back from the
atomic ioctl since that one is still associated with the original
scanout buffers.

The fence returned by the atomic ioctl will only signal after the
vblank, at which point userspace will obviously know that the
orginal scanout buffers are now also ready for reuse, and that
the last buffer submitted for each plane is now being actively
scanned out. So if userspace wants to send out some of the
original buffers to someone else, it can send along the fence
returned from the atomic ioctl.

So requires a bit more work from userspace perhaps. But obviously
it only has to do this if it plans on submitting multiple operations
per frame.

So a slightly expanded version of my previous example might look like:
starting point:
 plane a, fb 0
 plane b, fb 1

ioctl:
 crtc: fence A
 plane a, fb 2
 plane b, fb 3

ioctl:
 crtc: fence -1
 plane a, fb 4
 -> the previously submitted fb for plane a (fb 2) can be reused

ioctl:
 crtc: fence -1
 plane a, fb 5
 -> the previously submitted fb for plane a (fb 4) can be reused

vblank:
 -> fence A signals, fb 0,1 can be reused
fb 3 and 5 being scanned out now

We also had a quick side track w.r.t. variable refresh rate displays,
and I think the conclusion was that there the vblank may just happen
sooner or later. Nothing else should change. What's unclear is how
the refresh rate would be controlled. The two obvious options are
explicit control, and automagic based on the submit rate. But that's
a separate topic entirely.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v5 4/4] drm/fence: add out-fences support

2016-10-20 Thread Ville Syrjälä

On Thu, Oct 20, 2016 at 07:34:44PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 20, 2016 at 01:55:38PM -0200, Gustavo Padovan wrote:
> > 2016-10-20 Ville Syrjälä :
> > 
> > > On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
> > > > From: Gustavo Padovan 
> > > > 
> > > > Support DRM out-fences by creating a sync_file with a fence for each 
> > > > CRTC
> > > > that sets the OUT_FENCE_PTR property.
> > > 
> > > I still maintain the out fence should also be per fb (well, per plane
> > > since we can't add it to fbs). Otherwise it's not going to be at all
> > > useful if you do fps>vrefresh and don't include the same set of planes
> > > in every atomic ioctl, eg. if you only ever include ones that are
> > > somehow dirty.
> > 
> > How would the kernel signal these dirty planes then? Right now we signal
> > at the vblank.
> 
> So if I think about it in terms of the previous fbs something like this
> comes to mind:
> 
>  starting point:
>  plane a, fb 0
>  plane b, fb 1
> 
>  ioctl:
>  plane a, fb 2, fence A
>  plane b, fb 3, fence B
> 
>  ioctl:
>  plane a, fb 4, fence C
>  -> fb 2 can be reused, so fence C should signal immediately?
> 
>  vblank:
>  -> fb 0,1 can be reused, so fence A,B signal?
> 
> It feels a bit wonky since the fence is effectively associated with the
> previous fb after the previous fb was already submitted to the kernel.
> One might assume fence A to be the one signalled early, but that would
> give the impression that fb 0 would be free for reuse when it's not.

OK, so after hashing this out on irc with Gustavo and Brian, we came
to the conclusion that the per-crtc out fence should be sufficient
after all.

The way it can work is that the first ioctl will return the fence,
doesn't really matter how many of the planes are involved in this
ioctl. Subsequent ioctls that manage to get in before the next
vblank will get back a -1 as the fence, even if the set if planes
involved is not the same as in the first ioctl. From the -1
userspace can tell what happened, and can then consult its own
records to see which still pending flips were overwritten, and
thus knows which buffers can be reused immediately.

If userspace plans on sending out the now free buffers to someone
else accompanied by a fence, it can just create an already signalled
fence to send instead of sending the fence it got back from the
atomic ioctl since that one is still associated with the original
scanout buffers.

The fence returned by the atomic ioctl will only signal after the
vblank, at which point userspace will obviously know that the
orginal scanout buffers are now also ready for reuse, and that
the last buffer submitted for each plane is now being actively
scanned out. So if userspace wants to send out some of the
original buffers to someone else, it can send along the fence
returned from the atomic ioctl.

So requires a bit more work from userspace perhaps. But obviously
it only has to do this if it plans on submitting multiple operations
per frame.

So a slightly expanded version of my previous example might look like:
starting point:
 plane a, fb 0
 plane b, fb 1

ioctl:
 crtc: fence A
 plane a, fb 2
 plane b, fb 3

ioctl:
 crtc: fence -1
 plane a, fb 4
 -> the previously submitted fb for plane a (fb 2) can be reused

ioctl:
 crtc: fence -1
 plane a, fb 5
 -> the previously submitted fb for plane a (fb 4) can be reused

vblank:
 -> fence A signals, fb 0,1 can be reused
fb 3 and 5 being scanned out now

We also had a quick side track w.r.t. variable refresh rate displays,
and I think the conclusion was that there the vblank may just happen
sooner or later. Nothing else should change. What's unclear is how
the refresh rate would be controlled. The two obvious options are
explicit control, and automagic based on the submit rate. But that's
a separate topic entirely.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v5 4/4] drm/fence: add out-fences support

2016-10-20 Thread Brian Starkey


Hi Gustavo,

I notice your branch has the sync_file refcount change in, but this
doesn't seem to take account for that. Will you be dropping that
change to match the semantics of fence_array?

Couple more comments below.

On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:

From: Gustavo Padovan 

Support DRM out-fences by creating a sync_file with a fence for each CRTC
that sets the OUT_FENCE_PTR property.

We use the out_fence pointer received in the OUT_FENCE_PTR prop to send
the sync_file fd back to userspace.

The sync_file and fd are allocated/created before commit, but the
fd_install operation only happens after we know that commit succeed.

In case of error userspace should receive a fd number of -1.

v2: Comment by Rob Clark:
- Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here.

   Comment by Daniel Vetter:
- Add clean up code for out_fences

v3: Comments by Daniel Vetter:
- create DRM_MODE_ATOMIC_EVENT_MASK
- userspace should fill out_fences_ptr with the crtc_ids for which
it wants fences back.

v4: Create OUT_FENCE_PTR properties and remove old approach.

Signed-off-by: Gustavo Padovan 
---
drivers/gpu/drm/drm_atomic.c | 116 ++-
drivers/gpu/drm/drm_crtc.c   |   8 +++
include/drm/drm_crtc.h   |  19 +++
3 files changed, 119 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index b3284b2..07775fc 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -490,6 +490,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
);
state->color_mgmt_changed |= replaced;
return ret;
+   } else if (property == config->prop_out_fence_ptr) {
+   state->out_fence_ptr = u64_to_user_ptr(val);
} else if (crtc->funcs->atomic_set_property)
return crtc->funcs->atomic_set_property(crtc, state, property, 
val);
else
@@ -532,6 +534,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
*val = (state->ctm) ? state->ctm->base.id : 0;
else if (property == config->gamma_lut_property)
*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
+   else if (property == config->prop_out_fence_ptr)
+   *val = 0;
else if (crtc->funcs->atomic_get_property)
return crtc->funcs->atomic_get_property(crtc, state, property, 
val);
else
@@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
 */

static struct drm_pending_vblank_event *create_vblank_event(
-   struct drm_device *dev, struct drm_file *file_priv,
-   struct fence *fence, uint64_t user_data)
+   struct drm_device *dev, uint64_t user_data)
{
struct drm_pending_vblank_event *e = NULL;
-   int ret;

e = kzalloc(sizeof *e, GFP_KERNEL);
if (!e)
@@ -1488,17 +1490,6 @@ static struct drm_pending_vblank_event 
*create_vblank_event(
e->event.base.length = sizeof(e->event);
e->event.user_data = user_data;

-   if (file_priv) {
-   ret = drm_event_reserve_init(dev, file_priv, >base,
->event.base);
-   if (ret) {
-   kfree(e);
-   return NULL;
-   }
-   }
-
-   e->base.fence = fence;
-
return e;
}

@@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
}
EXPORT_SYMBOL(drm_atomic_clean_old_fb);

+static int crtc_setup_out_fence(struct drm_crtc *crtc,
+   struct drm_crtc_state *crtc_state,
+   struct drm_out_fence_state *fence_state)
+{
+   struct fence *fence;
+
+   fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
+   if (fence_state->fd < 0) {
+   return fence_state->fd;
+   }
+
+   fence_state->out_fence_ptr = crtc_state->out_fence_ptr;
+   crtc_state->out_fence_ptr = NULL;
+
+   if (put_user(fence_state->fd, fence_state->out_fence_ptr))
+   return -EFAULT;
+
+   fence = kzalloc(sizeof(*fence), GFP_KERNEL);
+   if (!fence)
+   return -ENOMEM;
+
+   fence_init(fence, _crtc_fence_ops, >fence_lock,
+  crtc->fence_context, ++crtc->fence_seqno);
+
+   fence_state->sync_file = sync_file_create(fence);
+   if(!fence_state->sync_file) {
+   fence_put(fence);
+   return -ENOMEM;
+   }
+
+   crtc_state->event->base.fence = fence_get(fence);
+   return 0;
+}
+
int drm_mode_atomic_ioctl(struct drm_device *dev,
  void *data, struct drm_file *file_priv)
{
@@ -1617,9 +1642,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
struct drm_plane *plane;
struct

Re: [PATCH v5 4/4] drm/fence: add out-fences support

2016-10-20 Thread Brian Starkey


Hi Gustavo,

I notice your branch has the sync_file refcount change in, but this
doesn't seem to take account for that. Will you be dropping that
change to match the semantics of fence_array?

Couple more comments below.

On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:

From: Gustavo Padovan 

Support DRM out-fences by creating a sync_file with a fence for each CRTC
that sets the OUT_FENCE_PTR property.

We use the out_fence pointer received in the OUT_FENCE_PTR prop to send
the sync_file fd back to userspace.

The sync_file and fd are allocated/created before commit, but the
fd_install operation only happens after we know that commit succeed.

In case of error userspace should receive a fd number of -1.

v2: Comment by Rob Clark:
- Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here.

   Comment by Daniel Vetter:
- Add clean up code for out_fences

v3: Comments by Daniel Vetter:
- create DRM_MODE_ATOMIC_EVENT_MASK
- userspace should fill out_fences_ptr with the crtc_ids for which
it wants fences back.

v4: Create OUT_FENCE_PTR properties and remove old approach.

Signed-off-by: Gustavo Padovan 
---
drivers/gpu/drm/drm_atomic.c | 116 ++-
drivers/gpu/drm/drm_crtc.c   |   8 +++
include/drm/drm_crtc.h   |  19 +++
3 files changed, 119 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index b3284b2..07775fc 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -490,6 +490,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
);
state->color_mgmt_changed |= replaced;
return ret;
+   } else if (property == config->prop_out_fence_ptr) {
+   state->out_fence_ptr = u64_to_user_ptr(val);
} else if (crtc->funcs->atomic_set_property)
return crtc->funcs->atomic_set_property(crtc, state, property, 
val);
else
@@ -532,6 +534,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
*val = (state->ctm) ? state->ctm->base.id : 0;
else if (property == config->gamma_lut_property)
*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
+   else if (property == config->prop_out_fence_ptr)
+   *val = 0;
else if (crtc->funcs->atomic_get_property)
return crtc->funcs->atomic_get_property(crtc, state, property, 
val);
else
@@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
 */

static struct drm_pending_vblank_event *create_vblank_event(
-   struct drm_device *dev, struct drm_file *file_priv,
-   struct fence *fence, uint64_t user_data)
+   struct drm_device *dev, uint64_t user_data)
{
struct drm_pending_vblank_event *e = NULL;
-   int ret;

e = kzalloc(sizeof *e, GFP_KERNEL);
if (!e)
@@ -1488,17 +1490,6 @@ static struct drm_pending_vblank_event 
*create_vblank_event(
e->event.base.length = sizeof(e->event);
e->event.user_data = user_data;

-   if (file_priv) {
-   ret = drm_event_reserve_init(dev, file_priv, >base,
->event.base);
-   if (ret) {
-   kfree(e);
-   return NULL;
-   }
-   }
-
-   e->base.fence = fence;
-
return e;
}

@@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
}
EXPORT_SYMBOL(drm_atomic_clean_old_fb);

+static int crtc_setup_out_fence(struct drm_crtc *crtc,
+   struct drm_crtc_state *crtc_state,
+   struct drm_out_fence_state *fence_state)
+{
+   struct fence *fence;
+
+   fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
+   if (fence_state->fd < 0) {
+   return fence_state->fd;
+   }
+
+   fence_state->out_fence_ptr = crtc_state->out_fence_ptr;
+   crtc_state->out_fence_ptr = NULL;
+
+   if (put_user(fence_state->fd, fence_state->out_fence_ptr))
+   return -EFAULT;
+
+   fence = kzalloc(sizeof(*fence), GFP_KERNEL);
+   if (!fence)
+   return -ENOMEM;
+
+   fence_init(fence, _crtc_fence_ops, >fence_lock,
+  crtc->fence_context, ++crtc->fence_seqno);
+
+   fence_state->sync_file = sync_file_create(fence);
+   if(!fence_state->sync_file) {
+   fence_put(fence);
+   return -ENOMEM;
+   }
+
+   crtc_state->event->base.fence = fence_get(fence);
+   return 0;
+}
+
int drm_mode_atomic_ioctl(struct drm_device *dev,
  void *data, struct drm_file *file_priv)
{
@@ -1617,9 +1642,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
struct drm_plane *plane;
struct drm_crtc *crtc;
struct drm_crtc_state *crtc_state;
+

Re: [PATCH v5 4/4] drm/fence: add out-fences support

2016-10-20 Thread Ville Syrjälä

On Thu, Oct 20, 2016 at 01:55:38PM -0200, Gustavo Padovan wrote:
> 2016-10-20 Ville Syrjälä :
> 
> > On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
> > > From: Gustavo Padovan 
> > > 
> > > Support DRM out-fences by creating a sync_file with a fence for each CRTC
> > > that sets the OUT_FENCE_PTR property.
> > 
> > I still maintain the out fence should also be per fb (well, per plane
> > since we can't add it to fbs). Otherwise it's not going to be at all
> > useful if you do fps>vrefresh and don't include the same set of planes
> > in every atomic ioctl, eg. if you only ever include ones that are
> > somehow dirty.
> 
> How would the kernel signal these dirty planes then? Right now we signal
> at the vblank.

So if I think about it in terms of the previous fbs something like this
comes to mind:

 starting point:
 plane a, fb 0
 plane b, fb 1

 ioctl:
 plane a, fb 2, fence A
 plane b, fb 3, fence B

 ioctl:
 plane a, fb 4, fence C
 -> fb 2 can be reused, so fence C should signal immediately?

 vblank:
 -> fb 0,1 can be reused, so fence A,B signal?

It feels a bit wonky since the fence is effectively associated with the
previous fb after the previous fb was already submitted to the kernel.
One might assume fence A to be the one signalled early, but that would
give the impression that fb 0 would be free for reuse when it's not.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v5 4/4] drm/fence: add out-fences support

2016-10-20 Thread Ville Syrjälä

On Thu, Oct 20, 2016 at 01:55:38PM -0200, Gustavo Padovan wrote:
> 2016-10-20 Ville Syrjälä :
> 
> > On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
> > > From: Gustavo Padovan 
> > > 
> > > Support DRM out-fences by creating a sync_file with a fence for each CRTC
> > > that sets the OUT_FENCE_PTR property.
> > 
> > I still maintain the out fence should also be per fb (well, per plane
> > since we can't add it to fbs). Otherwise it's not going to be at all
> > useful if you do fps>vrefresh and don't include the same set of planes
> > in every atomic ioctl, eg. if you only ever include ones that are
> > somehow dirty.
> 
> How would the kernel signal these dirty planes then? Right now we signal
> at the vblank.

So if I think about it in terms of the previous fbs something like this
comes to mind:

 starting point:
 plane a, fb 0
 plane b, fb 1

 ioctl:
 plane a, fb 2, fence A
 plane b, fb 3, fence B

 ioctl:
 plane a, fb 4, fence C
 -> fb 2 can be reused, so fence C should signal immediately?

 vblank:
 -> fb 0,1 can be reused, so fence A,B signal?

It feels a bit wonky since the fence is effectively associated with the
previous fb after the previous fb was already submitted to the kernel.
One might assume fence A to be the one signalled early, but that would
give the impression that fb 0 would be free for reuse when it's not.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v5 4/4] drm/fence: add out-fences support

2016-10-20 Thread Gustavo Padovan

2016-10-20 Ville Syrjälä :

> On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
> > From: Gustavo Padovan 
> > 
> > Support DRM out-fences by creating a sync_file with a fence for each CRTC
> > that sets the OUT_FENCE_PTR property.
> 
> I still maintain the out fence should also be per fb (well, per plane
> since we can't add it to fbs). Otherwise it's not going to be at all
> useful if you do fps>vrefresh and don't include the same set of planes
> in every atomic ioctl, eg. if you only ever include ones that are
> somehow dirty.

How would the kernel signal these dirty planes then? Right now we signal
at the vblank.

Gustavo

Re: [PATCH v5 4/4] drm/fence: add out-fences support

2016-10-20 Thread Gustavo Padovan

2016-10-20 Ville Syrjälä :

> On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
> > From: Gustavo Padovan 
> > 
> > Support DRM out-fences by creating a sync_file with a fence for each CRTC
> > that sets the OUT_FENCE_PTR property.
> 
> I still maintain the out fence should also be per fb (well, per plane
> since we can't add it to fbs). Otherwise it's not going to be at all
> useful if you do fps>vrefresh and don't include the same set of planes
> in every atomic ioctl, eg. if you only ever include ones that are
> somehow dirty.

How would the kernel signal these dirty planes then? Right now we signal
at the vblank.

Gustavo

Re: [PATCH v5 4/4] drm/fence: add out-fences support

2016-10-20 Thread Ville Syrjälä

On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
> From: Gustavo Padovan 
> 
> Support DRM out-fences by creating a sync_file with a fence for each CRTC
> that sets the OUT_FENCE_PTR property.

I still maintain the out fence should also be per fb (well, per plane
since we can't add it to fbs). Otherwise it's not going to be at all
useful if you do fps>vrefresh and don't include the same set of planes
in every atomic ioctl, eg. if you only ever include ones that are
somehow dirty.

> 
> We use the out_fence pointer received in the OUT_FENCE_PTR prop to send
> the sync_file fd back to userspace.
> 
> The sync_file and fd are allocated/created before commit, but the
> fd_install operation only happens after we know that commit succeed.
> 
> In case of error userspace should receive a fd number of -1.
> 
> v2: Comment by Rob Clark:
>   - Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here.
> 
> Comment by Daniel Vetter:
>   - Add clean up code for out_fences
> 
> v3: Comments by Daniel Vetter:
>   - create DRM_MODE_ATOMIC_EVENT_MASK
>   - userspace should fill out_fences_ptr with the crtc_ids for which
>   it wants fences back.
> 
> v4: Create OUT_FENCE_PTR properties and remove old approach.
> 
> Signed-off-by: Gustavo Padovan 
> ---
>  drivers/gpu/drm/drm_atomic.c | 116 
> ++-
>  drivers/gpu/drm/drm_crtc.c   |   8 +++
>  include/drm/drm_crtc.h   |  19 +++
>  3 files changed, 119 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index b3284b2..07775fc 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -490,6 +490,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>   );
>   state->color_mgmt_changed |= replaced;
>   return ret;
> + } else if (property == config->prop_out_fence_ptr) {
> + state->out_fence_ptr = u64_to_user_ptr(val);
>   } else if (crtc->funcs->atomic_set_property)
>   return crtc->funcs->atomic_set_property(crtc, state, property, 
> val);
>   else
> @@ -532,6 +534,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>   *val = (state->ctm) ? state->ctm->base.id : 0;
>   else if (property == config->gamma_lut_property)
>   *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
> + else if (property == config->prop_out_fence_ptr)
> + *val = 0;
>   else if (crtc->funcs->atomic_get_property)
>   return crtc->funcs->atomic_get_property(crtc, state, property, 
> val);
>   else
> @@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
>   */
>  
>  static struct drm_pending_vblank_event *create_vblank_event(
> - struct drm_device *dev, struct drm_file *file_priv,
> - struct fence *fence, uint64_t user_data)
> + struct drm_device *dev, uint64_t user_data)
>  {
>   struct drm_pending_vblank_event *e = NULL;
> - int ret;
>  
>   e = kzalloc(sizeof *e, GFP_KERNEL);
>   if (!e)
> @@ -1488,17 +1490,6 @@ static struct drm_pending_vblank_event 
> *create_vblank_event(
>   e->event.base.length = sizeof(e->event);
>   e->event.user_data = user_data;
>  
> - if (file_priv) {
> - ret = drm_event_reserve_init(dev, file_priv, >base,
> -  >event.base);
> - if (ret) {
> - kfree(e);
> - return NULL;
> - }
> - }
> -
> - e->base.fence = fence;
> -
>   return e;
>  }
>  
> @@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_atomic_clean_old_fb);
>  
> +static int crtc_setup_out_fence(struct drm_crtc *crtc,
> + struct drm_crtc_state *crtc_state,
> + struct drm_out_fence_state *fence_state)
> +{
> + struct fence *fence;
> +
> + fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
> + if (fence_state->fd < 0) {
> + return fence_state->fd;
> + }
> +
> + fence_state->out_fence_ptr = crtc_state->out_fence_ptr;
> + crtc_state->out_fence_ptr = NULL;
> +
> + if (put_user(fence_state->fd, fence_state->out_fence_ptr))
> + return -EFAULT;
> +
> + fence = kzalloc(sizeof(*fence), GFP_KERNEL);
> + if (!fence)
> + return -ENOMEM;
> +
> + fence_init(fence, _crtc_fence_ops, >fence_lock,
> +crtc->fence_context, ++crtc->fence_seqno);
> +
> + fence_state->sync_file = sync_file_create(fence);
> + if(!fence_state->sync_file) {
> + fence_put(fence);
> + return -ENOMEM;
> + }
> +
> + crtc_state->event->base.fence = fence_get(fence);
> + return 0;
> +}
> +
>  int

Re: [PATCH v5 4/4] drm/fence: add out-fences support

2016-10-20 Thread Ville Syrjälä

On Thu, Oct 20, 2016 at 12:50:05PM -0200, Gustavo Padovan wrote:
> From: Gustavo Padovan 
> 
> Support DRM out-fences by creating a sync_file with a fence for each CRTC
> that sets the OUT_FENCE_PTR property.

I still maintain the out fence should also be per fb (well, per plane
since we can't add it to fbs). Otherwise it's not going to be at all
useful if you do fps>vrefresh and don't include the same set of planes
in every atomic ioctl, eg. if you only ever include ones that are
somehow dirty.

> 
> We use the out_fence pointer received in the OUT_FENCE_PTR prop to send
> the sync_file fd back to userspace.
> 
> The sync_file and fd are allocated/created before commit, but the
> fd_install operation only happens after we know that commit succeed.
> 
> In case of error userspace should receive a fd number of -1.
> 
> v2: Comment by Rob Clark:
>   - Squash commit that adds DRM_MODE_ATOMIC_OUT_FENCE flag here.
> 
> Comment by Daniel Vetter:
>   - Add clean up code for out_fences
> 
> v3: Comments by Daniel Vetter:
>   - create DRM_MODE_ATOMIC_EVENT_MASK
>   - userspace should fill out_fences_ptr with the crtc_ids for which
>   it wants fences back.
> 
> v4: Create OUT_FENCE_PTR properties and remove old approach.
> 
> Signed-off-by: Gustavo Padovan 
> ---
>  drivers/gpu/drm/drm_atomic.c | 116 
> ++-
>  drivers/gpu/drm/drm_crtc.c   |   8 +++
>  include/drm/drm_crtc.h   |  19 +++
>  3 files changed, 119 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index b3284b2..07775fc 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -490,6 +490,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>   );
>   state->color_mgmt_changed |= replaced;
>   return ret;
> + } else if (property == config->prop_out_fence_ptr) {
> + state->out_fence_ptr = u64_to_user_ptr(val);
>   } else if (crtc->funcs->atomic_set_property)
>   return crtc->funcs->atomic_set_property(crtc, state, property, 
> val);
>   else
> @@ -532,6 +534,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>   *val = (state->ctm) ? state->ctm->base.id : 0;
>   else if (property == config->gamma_lut_property)
>   *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
> + else if (property == config->prop_out_fence_ptr)
> + *val = 0;
>   else if (crtc->funcs->atomic_get_property)
>   return crtc->funcs->atomic_get_property(crtc, state, property, 
> val);
>   else
> @@ -1474,11 +1478,9 @@ EXPORT_SYMBOL(drm_atomic_nonblocking_commit);
>   */
>  
>  static struct drm_pending_vblank_event *create_vblank_event(
> - struct drm_device *dev, struct drm_file *file_priv,
> - struct fence *fence, uint64_t user_data)
> + struct drm_device *dev, uint64_t user_data)
>  {
>   struct drm_pending_vblank_event *e = NULL;
> - int ret;
>  
>   e = kzalloc(sizeof *e, GFP_KERNEL);
>   if (!e)
> @@ -1488,17 +1490,6 @@ static struct drm_pending_vblank_event 
> *create_vblank_event(
>   e->event.base.length = sizeof(e->event);
>   e->event.user_data = user_data;
>  
> - if (file_priv) {
> - ret = drm_event_reserve_init(dev, file_priv, >base,
> -  >event.base);
> - if (ret) {
> - kfree(e);
> - return NULL;
> - }
> - }
> -
> - e->base.fence = fence;
> -
>   return e;
>  }
>  
> @@ -1603,6 +1594,40 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_atomic_clean_old_fb);
>  
> +static int crtc_setup_out_fence(struct drm_crtc *crtc,
> + struct drm_crtc_state *crtc_state,
> + struct drm_out_fence_state *fence_state)
> +{
> + struct fence *fence;
> +
> + fence_state->fd = get_unused_fd_flags(O_CLOEXEC);
> + if (fence_state->fd < 0) {
> + return fence_state->fd;
> + }
> +
> + fence_state->out_fence_ptr = crtc_state->out_fence_ptr;
> + crtc_state->out_fence_ptr = NULL;
> +
> + if (put_user(fence_state->fd, fence_state->out_fence_ptr))
> + return -EFAULT;
> +
> + fence = kzalloc(sizeof(*fence), GFP_KERNEL);
> + if (!fence)
> + return -ENOMEM;
> +
> + fence_init(fence, _crtc_fence_ops, >fence_lock,
> +crtc->fence_context, ++crtc->fence_seqno);
> +
> + fence_state->sync_file = sync_file_create(fence);
> + if(!fence_state->sync_file) {
> + fence_put(fence);
> + return -ENOMEM;
> + }
> +
> + crtc_state->event->base.fence = fence_get(fence);
> + return 0;
> +}
> +
>  int drm_mode_atomic_ioctl(struct drm_device *dev,
> void *data,

Re: [PATCH v5 4/4] drm/fence: add out-fences support

Re: [PATCH v5 4/4] drm/fence: add out-fences support

Re: [PATCH v5 4/4] drm/fence: add out-fences support

Re: [PATCH v5 4/4] drm/fence: add out-fences support

Re: [PATCH v5 4/4] drm/fence: add out-fences support

Re: [PATCH v5 4/4] drm/fence: add out-fences support

Re: [PATCH v5 4/4] drm/fence: add out-fences support

Re: [PATCH v5 4/4] drm/fence: add out-fences support

Re: [PATCH v5 4/4] drm/fence: add out-fences support

Re: [PATCH v5 4/4] drm/fence: add out-fences support

Re: [PATCH v5 4/4] drm/fence: add out-fences support

Re: [PATCH v5 4/4] drm/fence: add out-fences support

Re: [PATCH v5 4/4] drm/fence: add out-fences support

Re: [PATCH v5 4/4] drm/fence: add out-fences support

Re: [PATCH v5 4/4] drm/fence: add out-fences support

Re: [PATCH v5 4/4] drm/fence: add out-fences support

Re: [PATCH v5 4/4] drm/fence: add out-fences support

Re: [PATCH v5 4/4] drm/fence: add out-fences support

Re: [PATCH v5 4/4] drm/fence: add out-fences support

Re: [PATCH v5 4/4] drm/fence: add out-fences support

Re: [PATCH v5 4/4] drm/fence: add out-fences support

Re: [PATCH v5 4/4] drm/fence: add out-fences support

22 matches

Site Navigation

Mail list logo

Footer information