Re: [RFC 2/3] drm/atomic: Call dma_fence_boost() when we've missed a vblank
On Tue, Jun 01, 2021 at 08:46:14AM -0700, Rob Clark wrote:
> On Tue, Jun 1, 2021 at 7:18 AM Daniel Vetter wrote:
> >
> > On Sun, May 30, 2021 at 07:33:57AM -0700, Rob Clark wrote:
> > > On Thu, May 20, 2021 at 9:29 AM Daniel Vetter wrote:
> > > >
> > > > On Wed, May 19, 2021 at 11:38:53AM -0700, Rob Clark wrote:
> > > > > From: Rob Clark
> > > > >
> > > > > Signed-off-by: Rob Clark
> > > > > ---
> > > > > drivers/gpu/drm/drm_atomic_helper.c | 11 +++
> > > > > 1 file changed, 11 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > index 560aaecba31b..fe10fc2e7f86 100644
> > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > @@ -1435,11 +1435,15 @@ int drm_atomic_helper_wait_for_fences(struct
> > > > > drm_device *dev,
> > > > > int i, ret;
> > > > >
> > > > > for_each_new_plane_in_state(state, plane, new_plane_state, i) {
> > > > > + u64 vblank_count;
> > > > > +
> > > > > if (!new_plane_state->fence)
> > > > > continue;
> > > > >
> > > > > WARN_ON(!new_plane_state->fb);
> > > > >
> > > > > + vblank_count =
> > > > > drm_crtc_vblank_count(new_plane_state->crtc);
> > > > > +
> > > > > /*
> > > > >* If waiting for fences pre-swap (ie: nonblock),
> > > > > userspace can
> > > > >* still interrupt the operation. Instead of blocking
> > > > > until the
> > > > > @@ -1449,6 +1453,13 @@ int drm_atomic_helper_wait_for_fences(struct
> > > > > drm_device *dev,
> > > > > if (ret)
> > > > > return ret;
> > > > >
> > > > > + /*
> > > > > + * Check if we've missed a vblank while waiting, and if
> > > > > we have
> > > > > + * signal the fence that it's signaler should be boosted
> > > > > + */
> > > > > + if (vblank_count !=
> > > > > drm_crtc_vblank_count(new_plane_state->crtc))
> > > > > + dma_fence_boost(new_plane_state->fence);
> > > >
> > > > I think we should do a lot better here:
> > > > - maybe only bother doing this for single-crtc updates, and only if
> > > > modeset isn't set. No one else cares about latency.
> > > >
> > > > - We should boost _right_ when we've missed the frame, so I think we
> > > > should have a _timeout wait here that guesstimates when the vblank is
> > > > over (might need to throw in a vblank wait if we missed) and then
> > > > boost
> > > > immediately. Not wait a bunch of frames (worst case) until we finally
> > > > decide to boost.
> > >
> > > I was thinking about this a bit more.. How about rather than calling
> > > some fence->op->boost() type thing when we are about to miss a vblank
> > > (IMO that is also already too late), we do something more like
> > > fence->ops->set_deadline() before we even wait?
> >
> > Hm yeah that sounds like a clean idea.
> >
> > Even more, why not add the deadline/waiter information to the callback
> > we're adding? That way drivers can inspect it whenever they feel like and
> > don't have to duplicate the tracking. And it's probably easier to
> > tune/adjust to the myriads of use-cases (flip target miss, userspace wait,
> > wakeup boost maybe too ...).
>
> You mean, enumerate the types of deadline?
>
> For userspace waits, we might have a timeout, but not really
> (currently) any more information than that? The vblank deadline is
> the only type of deadline that seems pretty clear to me.
>
> I suppose we could do something like:
>
>dma_fence_set_deadline(fence, &(struct dma_fence_deadline){
>.type = DMA_FENCE_DEADLINE_VBLANK,
>.time = next_vblank_ktime,
>});
>
> to make it a bit more extensible to add more deadline types or
> additional optional information
Nah not enumerate the types of deadlines, but the types of waits. Some of
which might have a deadline (like page flip), some wont (like userspace
waiting or poll() on a dma-fd or whatever).
What I had in mind is roughly
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 6ffb4b2c6371..e7c239145273 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -116,6 +116,8 @@ typedef void (*dma_fence_func_t)(struct dma_fence *fence,
struct dma_fence_cb {
struct list_head node;
dma_fence_func_t func;
+ enume dma_fence_wait_type wait_type;
+ struct ktime deadline; /* fixme how do we indicate no deadline? */
};
/**
With that waiters, and irrespective of whether they use dma_fence_wait or
have something else like the dma-buf fd poll stuff, can indicate to the
driver what kind of wait with what kind of deadline this is.
Maybe we should make this a sub-struct, so that it can also be passed to
dma_fence_wait().
-Daniel
>
> BR,
> -R
>
> >
> > I like this direction a lot
Re: [RFC 2/3] drm/atomic: Call dma_fence_boost() when we've missed a vblank
On Tue, Jun 1, 2021 at 7:18 AM Daniel Vetter wrote:
>
> On Sun, May 30, 2021 at 07:33:57AM -0700, Rob Clark wrote:
> > On Thu, May 20, 2021 at 9:29 AM Daniel Vetter wrote:
> > >
> > > On Wed, May 19, 2021 at 11:38:53AM -0700, Rob Clark wrote:
> > > > From: Rob Clark
> > > >
> > > > Signed-off-by: Rob Clark
> > > > ---
> > > > drivers/gpu/drm/drm_atomic_helper.c | 11 +++
> > > > 1 file changed, 11 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > > b/drivers/gpu/drm/drm_atomic_helper.c
> > > > index 560aaecba31b..fe10fc2e7f86 100644
> > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > @@ -1435,11 +1435,15 @@ int drm_atomic_helper_wait_for_fences(struct
> > > > drm_device *dev,
> > > > int i, ret;
> > > >
> > > > for_each_new_plane_in_state(state, plane, new_plane_state, i) {
> > > > + u64 vblank_count;
> > > > +
> > > > if (!new_plane_state->fence)
> > > > continue;
> > > >
> > > > WARN_ON(!new_plane_state->fb);
> > > >
> > > > + vblank_count =
> > > > drm_crtc_vblank_count(new_plane_state->crtc);
> > > > +
> > > > /*
> > > >* If waiting for fences pre-swap (ie: nonblock),
> > > > userspace can
> > > >* still interrupt the operation. Instead of blocking
> > > > until the
> > > > @@ -1449,6 +1453,13 @@ int drm_atomic_helper_wait_for_fences(struct
> > > > drm_device *dev,
> > > > if (ret)
> > > > return ret;
> > > >
> > > > + /*
> > > > + * Check if we've missed a vblank while waiting, and if
> > > > we have
> > > > + * signal the fence that it's signaler should be boosted
> > > > + */
> > > > + if (vblank_count !=
> > > > drm_crtc_vblank_count(new_plane_state->crtc))
> > > > + dma_fence_boost(new_plane_state->fence);
> > >
> > > I think we should do a lot better here:
> > > - maybe only bother doing this for single-crtc updates, and only if
> > > modeset isn't set. No one else cares about latency.
> > >
> > > - We should boost _right_ when we've missed the frame, so I think we
> > > should have a _timeout wait here that guesstimates when the vblank is
> > > over (might need to throw in a vblank wait if we missed) and then boost
> > > immediately. Not wait a bunch of frames (worst case) until we finally
> > > decide to boost.
> >
> > I was thinking about this a bit more.. How about rather than calling
> > some fence->op->boost() type thing when we are about to miss a vblank
> > (IMO that is also already too late), we do something more like
> > fence->ops->set_deadline() before we even wait?
>
> Hm yeah that sounds like a clean idea.
>
> Even more, why not add the deadline/waiter information to the callback
> we're adding? That way drivers can inspect it whenever they feel like and
> don't have to duplicate the tracking. And it's probably easier to
> tune/adjust to the myriads of use-cases (flip target miss, userspace wait,
> wakeup boost maybe too ...).
You mean, enumerate the types of deadline?
For userspace waits, we might have a timeout, but not really
(currently) any more information than that? The vblank deadline is
the only type of deadline that seems pretty clear to me.
I suppose we could do something like:
dma_fence_set_deadline(fence, &(struct dma_fence_deadline){
.type = DMA_FENCE_DEADLINE_VBLANK,
.time = next_vblank_ktime,
});
to make it a bit more extensible to add more deadline types or
additional optional information
BR,
-R
>
> I like this direction a lot more than what we discussed with post-miss
> hints thus far.
>
> > It's probably a bit impossible for a gpu driver to really predict how
> > long some rendering will take, but other cases like video decoder are
> > somewhat more predictable.. the fence provider could predict given the
> > remaining time until the deadline what clk rates are required to get
> > you there.
>
> Well if we do have a deadline the driver can note that in its scheduler
> and arm a driver to kick the clocks. Or maybe use past history to do this
> upfront.
> -Daniel
>
> >
> > BR,
> > -R
> >
> >
> > >
> > > Otherwise I really like this, I think it's about the only real reason i915
> > > isn't using atomic helpers.
> > >
> > > Also adding Matt B for this topic.
> > > -Daniel
> > >
> > > > +
> > > > dma_fence_put(new_plane_state->fence);
> > > > new_plane_state->fence = NULL;
> > > > }
> > > > --
> > > > 2.30.2
> > > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Re: [RFC 2/3] drm/atomic: Call dma_fence_boost() when we've missed a vblank
On Sun, May 30, 2021 at 07:33:57AM -0700, Rob Clark wrote:
> On Thu, May 20, 2021 at 9:29 AM Daniel Vetter wrote:
> >
> > On Wed, May 19, 2021 at 11:38:53AM -0700, Rob Clark wrote:
> > > From: Rob Clark
> > >
> > > Signed-off-by: Rob Clark
> > > ---
> > > drivers/gpu/drm/drm_atomic_helper.c | 11 +++
> > > 1 file changed, 11 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 560aaecba31b..fe10fc2e7f86 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -1435,11 +1435,15 @@ int drm_atomic_helper_wait_for_fences(struct
> > > drm_device *dev,
> > > int i, ret;
> > >
> > > for_each_new_plane_in_state(state, plane, new_plane_state, i) {
> > > + u64 vblank_count;
> > > +
> > > if (!new_plane_state->fence)
> > > continue;
> > >
> > > WARN_ON(!new_plane_state->fb);
> > >
> > > + vblank_count = drm_crtc_vblank_count(new_plane_state->crtc);
> > > +
> > > /*
> > >* If waiting for fences pre-swap (ie: nonblock), userspace
> > > can
> > >* still interrupt the operation. Instead of blocking until
> > > the
> > > @@ -1449,6 +1453,13 @@ int drm_atomic_helper_wait_for_fences(struct
> > > drm_device *dev,
> > > if (ret)
> > > return ret;
> > >
> > > + /*
> > > + * Check if we've missed a vblank while waiting, and if we
> > > have
> > > + * signal the fence that it's signaler should be boosted
> > > + */
> > > + if (vblank_count !=
> > > drm_crtc_vblank_count(new_plane_state->crtc))
> > > + dma_fence_boost(new_plane_state->fence);
> >
> > I think we should do a lot better here:
> > - maybe only bother doing this for single-crtc updates, and only if
> > modeset isn't set. No one else cares about latency.
> >
> > - We should boost _right_ when we've missed the frame, so I think we
> > should have a _timeout wait here that guesstimates when the vblank is
> > over (might need to throw in a vblank wait if we missed) and then boost
> > immediately. Not wait a bunch of frames (worst case) until we finally
> > decide to boost.
>
> I was thinking about this a bit more.. How about rather than calling
> some fence->op->boost() type thing when we are about to miss a vblank
> (IMO that is also already too late), we do something more like
> fence->ops->set_deadline() before we even wait?
Hm yeah that sounds like a clean idea.
Even more, why not add the deadline/waiter information to the callback
we're adding? That way drivers can inspect it whenever they feel like and
don't have to duplicate the tracking. And it's probably easier to
tune/adjust to the myriads of use-cases (flip target miss, userspace wait,
wakeup boost maybe too ...).
I like this direction a lot more than what we discussed with post-miss
hints thus far.
> It's probably a bit impossible for a gpu driver to really predict how
> long some rendering will take, but other cases like video decoder are
> somewhat more predictable.. the fence provider could predict given the
> remaining time until the deadline what clk rates are required to get
> you there.
Well if we do have a deadline the driver can note that in its scheduler
and arm a driver to kick the clocks. Or maybe use past history to do this
upfront.
-Daniel
>
> BR,
> -R
>
>
> >
> > Otherwise I really like this, I think it's about the only real reason i915
> > isn't using atomic helpers.
> >
> > Also adding Matt B for this topic.
> > -Daniel
> >
> > > +
> > > dma_fence_put(new_plane_state->fence);
> > > new_plane_state->fence = NULL;
> > > }
> > > --
> > > 2.30.2
> > >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: [RFC 2/3] drm/atomic: Call dma_fence_boost() when we've missed a vblank
On Thu, May 20, 2021 at 9:29 AM Daniel Vetter wrote:
>
> On Wed, May 19, 2021 at 11:38:53AM -0700, Rob Clark wrote:
> > From: Rob Clark
> >
> > Signed-off-by: Rob Clark
> > ---
> > drivers/gpu/drm/drm_atomic_helper.c | 11 +++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > b/drivers/gpu/drm/drm_atomic_helper.c
> > index 560aaecba31b..fe10fc2e7f86 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1435,11 +1435,15 @@ int drm_atomic_helper_wait_for_fences(struct
> > drm_device *dev,
> > int i, ret;
> >
> > for_each_new_plane_in_state(state, plane, new_plane_state, i) {
> > + u64 vblank_count;
> > +
> > if (!new_plane_state->fence)
> > continue;
> >
> > WARN_ON(!new_plane_state->fb);
> >
> > + vblank_count = drm_crtc_vblank_count(new_plane_state->crtc);
> > +
> > /*
> >* If waiting for fences pre-swap (ie: nonblock), userspace
> > can
> >* still interrupt the operation. Instead of blocking until
> > the
> > @@ -1449,6 +1453,13 @@ int drm_atomic_helper_wait_for_fences(struct
> > drm_device *dev,
> > if (ret)
> > return ret;
> >
> > + /*
> > + * Check if we've missed a vblank while waiting, and if we
> > have
> > + * signal the fence that it's signaler should be boosted
> > + */
> > + if (vblank_count !=
> > drm_crtc_vblank_count(new_plane_state->crtc))
> > + dma_fence_boost(new_plane_state->fence);
>
> I think we should do a lot better here:
> - maybe only bother doing this for single-crtc updates, and only if
> modeset isn't set. No one else cares about latency.
>
> - We should boost _right_ when we've missed the frame, so I think we
> should have a _timeout wait here that guesstimates when the vblank is
> over (might need to throw in a vblank wait if we missed) and then boost
> immediately. Not wait a bunch of frames (worst case) until we finally
> decide to boost.
I was thinking about this a bit more.. How about rather than calling
some fence->op->boost() type thing when we are about to miss a vblank
(IMO that is also already too late), we do something more like
fence->ops->set_deadline() before we even wait?
It's probably a bit impossible for a gpu driver to really predict how
long some rendering will take, but other cases like video decoder are
somewhat more predictable.. the fence provider could predict given the
remaining time until the deadline what clk rates are required to get
you there.
BR,
-R
>
> Otherwise I really like this, I think it's about the only real reason i915
> isn't using atomic helpers.
>
> Also adding Matt B for this topic.
> -Daniel
>
> > +
> > dma_fence_put(new_plane_state->fence);
> > new_plane_state->fence = NULL;
> > }
> > --
> > 2.30.2
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Re: [RFC 2/3] drm/atomic: Call dma_fence_boost() when we've missed a vblank
On Wed, May 19, 2021 at 11:38:53AM -0700, Rob Clark wrote:
> From: Rob Clark
>
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 560aaecba31b..fe10fc2e7f86 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1435,11 +1435,15 @@ int drm_atomic_helper_wait_for_fences(struct
> drm_device *dev,
> int i, ret;
>
> for_each_new_plane_in_state(state, plane, new_plane_state, i) {
> + u64 vblank_count;
> +
> if (!new_plane_state->fence)
> continue;
>
> WARN_ON(!new_plane_state->fb);
>
> + vblank_count = drm_crtc_vblank_count(new_plane_state->crtc);
> +
> /*
>* If waiting for fences pre-swap (ie: nonblock), userspace can
>* still interrupt the operation. Instead of blocking until the
> @@ -1449,6 +1453,13 @@ int drm_atomic_helper_wait_for_fences(struct
> drm_device *dev,
> if (ret)
> return ret;
>
> + /*
> + * Check if we've missed a vblank while waiting, and if we have
> + * signal the fence that it's signaler should be boosted
> + */
> + if (vblank_count !=
> drm_crtc_vblank_count(new_plane_state->crtc))
> + dma_fence_boost(new_plane_state->fence);
I think we should do a lot better here:
- maybe only bother doing this for single-crtc updates, and only if
modeset isn't set. No one else cares about latency.
- We should boost _right_ when we've missed the frame, so I think we
should have a _timeout wait here that guesstimates when the vblank is
over (might need to throw in a vblank wait if we missed) and then boost
immediately. Not wait a bunch of frames (worst case) until we finally
decide to boost.
Otherwise I really like this, I think it's about the only real reason i915
isn't using atomic helpers.
Also adding Matt B for this topic.
-Daniel
> +
> dma_fence_put(new_plane_state->fence);
> new_plane_state->fence = NULL;
> }
> --
> 2.30.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[RFC 2/3] drm/atomic: Call dma_fence_boost() when we've missed a vblank
From: Rob Clark
Signed-off-by: Rob Clark
---
drivers/gpu/drm/drm_atomic_helper.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index 560aaecba31b..fe10fc2e7f86 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1435,11 +1435,15 @@ int drm_atomic_helper_wait_for_fences(struct drm_device
*dev,
int i, ret;
for_each_new_plane_in_state(state, plane, new_plane_state, i) {
+ u64 vblank_count;
+
if (!new_plane_state->fence)
continue;
WARN_ON(!new_plane_state->fb);
+ vblank_count = drm_crtc_vblank_count(new_plane_state->crtc);
+
/*
* If waiting for fences pre-swap (ie: nonblock), userspace can
* still interrupt the operation. Instead of blocking until the
@@ -1449,6 +1453,13 @@ int drm_atomic_helper_wait_for_fences(struct drm_device
*dev,
if (ret)
return ret;
+ /*
+* Check if we've missed a vblank while waiting, and if we have
+* signal the fence that it's signaler should be boosted
+*/
+ if (vblank_count !=
drm_crtc_vblank_count(new_plane_state->crtc))
+ dma_fence_boost(new_plane_state->fence);
+
dma_fence_put(new_plane_state->fence);
new_plane_state->fence = NULL;
}
--
2.30.2
