Message ID | 20210807183804.459850-5-robdclark@gmail.com |
---|---|
State | New |
Headers | show |
Series | [v2,1/5] dma-fence: Add deadline awareness | expand |
Am 07.08.21 um 20:37 schrieb Rob Clark: > From: Rob Clark <robdclark@chromium.org> > > As the finished fence is the one that is exposed to userspace, and > therefore the one that other operations, like atomic update, would > block on, we need to propagate the deadline from from the finished > fence to the actual hw fence. > > Signed-off-by: Rob Clark <robdclark@chromium.org> > --- > drivers/gpu/drm/scheduler/sched_fence.c | 25 +++++++++++++++++++++++++ > drivers/gpu/drm/scheduler/sched_main.c | 3 +++ > include/drm/gpu_scheduler.h | 6 ++++++ > 3 files changed, 34 insertions(+) > > diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c > index 69de2c76731f..f389dca44185 100644 > --- a/drivers/gpu/drm/scheduler/sched_fence.c > +++ b/drivers/gpu/drm/scheduler/sched_fence.c > @@ -128,6 +128,30 @@ static void drm_sched_fence_release_finished(struct dma_fence *f) > dma_fence_put(&fence->scheduled); > } > > +static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, > + ktime_t deadline) > +{ > + struct drm_sched_fence *fence = to_drm_sched_fence(f); > + unsigned long flags; > + > + spin_lock_irqsave(&fence->lock, flags); > + > + /* If we already have an earlier deadline, keep it: */ > + if (test_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && > + ktime_before(fence->deadline, deadline)) { > + spin_unlock_irqrestore(&fence->lock, flags); > + return; > + } > + > + fence->deadline = deadline; > + set_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags); > + > + spin_unlock_irqrestore(&fence->lock, flags); > + > + if (fence->parent) > + dma_fence_set_deadline(fence->parent, deadline); > +} > + > static const struct dma_fence_ops drm_sched_fence_ops_scheduled = { > .get_driver_name = drm_sched_fence_get_driver_name, > .get_timeline_name = drm_sched_fence_get_timeline_name, > @@ -138,6 +162,7 @@ static const struct dma_fence_ops drm_sched_fence_ops_finished = { > .get_driver_name = drm_sched_fence_get_driver_name, > .get_timeline_name = drm_sched_fence_get_timeline_name, > .release = drm_sched_fence_release_finished, > + .set_deadline = drm_sched_fence_set_deadline_finished, > }; > > struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f) > diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c > index a2a953693b45..3ab0900d3596 100644 > --- a/drivers/gpu/drm/scheduler/sched_main.c > +++ b/drivers/gpu/drm/scheduler/sched_main.c > @@ -818,6 +818,9 @@ static int drm_sched_main(void *param) > > if (!IS_ERR_OR_NULL(fence)) { > s_fence->parent = dma_fence_get(fence); > + if (test_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT, > + &s_fence->finished.flags)) > + dma_fence_set_deadline(fence, s_fence->deadline); Maybe move this into a dma_sched_fence_set_parent() function. Apart from that looks good to me. Regards, Christian. > r = dma_fence_add_callback(fence, &sched_job->cb, > drm_sched_job_done_cb); > if (r == -ENOENT) > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h > index d18af49fd009..0f08ade614ae 100644 > --- a/include/drm/gpu_scheduler.h > +++ b/include/drm/gpu_scheduler.h > @@ -144,6 +144,12 @@ struct drm_sched_fence { > */ > struct dma_fence finished; > > + /** > + * @deadline: deadline set on &drm_sched_fence.finished which > + * potentially needs to be propagated to &drm_sched_fence.parent > + */ > + ktime_t deadline; > + > /** > * @parent: the fence returned by &drm_sched_backend_ops.run_job > * when scheduling the job on hardware. We signal the
On Mon, Aug 16, 2021 at 12:14:35PM +0200, Christian König wrote: > Am 07.08.21 um 20:37 schrieb Rob Clark: > > From: Rob Clark <robdclark@chromium.org> > > > > As the finished fence is the one that is exposed to userspace, and > > therefore the one that other operations, like atomic update, would > > block on, we need to propagate the deadline from from the finished > > fence to the actual hw fence. > > > > Signed-off-by: Rob Clark <robdclark@chromium.org> I guess you're already letting the compositor run at a higher gpu priority so that your deadline'd drm_sched_job isn't stuck behind the app rendering the next frame? I'm not sure whether you wire that one up as part of the conversion to drm/sched. Without that I think we might need to ponder how we can do a prio-boost for these, e.g. within a scheduling class we pick the jobs with the nearest deadline first, before we pick others. -Daniel > > --- > > drivers/gpu/drm/scheduler/sched_fence.c | 25 +++++++++++++++++++++++++ > > drivers/gpu/drm/scheduler/sched_main.c | 3 +++ > > include/drm/gpu_scheduler.h | 6 ++++++ > > 3 files changed, 34 insertions(+) > > > > diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c > > index 69de2c76731f..f389dca44185 100644 > > --- a/drivers/gpu/drm/scheduler/sched_fence.c > > +++ b/drivers/gpu/drm/scheduler/sched_fence.c > > @@ -128,6 +128,30 @@ static void drm_sched_fence_release_finished(struct dma_fence *f) > > dma_fence_put(&fence->scheduled); > > } > > +static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, > > + ktime_t deadline) > > +{ > > + struct drm_sched_fence *fence = to_drm_sched_fence(f); > > + unsigned long flags; > > + > > + spin_lock_irqsave(&fence->lock, flags); > > + > > + /* If we already have an earlier deadline, keep it: */ > > + if (test_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && > > + ktime_before(fence->deadline, deadline)) { > > + spin_unlock_irqrestore(&fence->lock, flags); > > + return; > > + } > > + > > + fence->deadline = deadline; > > + set_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags); > > + > > + spin_unlock_irqrestore(&fence->lock, flags); > > + > > + if (fence->parent) > > + dma_fence_set_deadline(fence->parent, deadline); > > +} > > + > > static const struct dma_fence_ops drm_sched_fence_ops_scheduled = { > > .get_driver_name = drm_sched_fence_get_driver_name, > > .get_timeline_name = drm_sched_fence_get_timeline_name, > > @@ -138,6 +162,7 @@ static const struct dma_fence_ops drm_sched_fence_ops_finished = { > > .get_driver_name = drm_sched_fence_get_driver_name, > > .get_timeline_name = drm_sched_fence_get_timeline_name, > > .release = drm_sched_fence_release_finished, > > + .set_deadline = drm_sched_fence_set_deadline_finished, > > }; > > struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f) > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c > > index a2a953693b45..3ab0900d3596 100644 > > --- a/drivers/gpu/drm/scheduler/sched_main.c > > +++ b/drivers/gpu/drm/scheduler/sched_main.c > > @@ -818,6 +818,9 @@ static int drm_sched_main(void *param) > > if (!IS_ERR_OR_NULL(fence)) { > > s_fence->parent = dma_fence_get(fence); > > + if (test_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT, > > + &s_fence->finished.flags)) > > + dma_fence_set_deadline(fence, s_fence->deadline); > > Maybe move this into a dma_sched_fence_set_parent() function. > > Apart from that looks good to me. > > Regards, > Christian. > > > r = dma_fence_add_callback(fence, &sched_job->cb, > > drm_sched_job_done_cb); > > if (r == -ENOENT) > > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h > > index d18af49fd009..0f08ade614ae 100644 > > --- a/include/drm/gpu_scheduler.h > > +++ b/include/drm/gpu_scheduler.h > > @@ -144,6 +144,12 @@ struct drm_sched_fence { > > */ > > struct dma_fence finished; > > + /** > > + * @deadline: deadline set on &drm_sched_fence.finished which > > + * potentially needs to be propagated to &drm_sched_fence.parent > > + */ > > + ktime_t deadline; > > + > > /** > > * @parent: the fence returned by &drm_sched_backend_ops.run_job > > * when scheduling the job on hardware. We signal the >
On Mon, Aug 16, 2021 at 8:38 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Mon, Aug 16, 2021 at 12:14:35PM +0200, Christian König wrote: > > Am 07.08.21 um 20:37 schrieb Rob Clark: > > > From: Rob Clark <robdclark@chromium.org> > > > > > > As the finished fence is the one that is exposed to userspace, and > > > therefore the one that other operations, like atomic update, would > > > block on, we need to propagate the deadline from from the finished > > > fence to the actual hw fence. > > > > > > Signed-off-by: Rob Clark <robdclark@chromium.org> > > I guess you're already letting the compositor run at a higher gpu priority > so that your deadline'd drm_sched_job isn't stuck behind the app rendering > the next frame? With the scheduler conversion we do have multiple priorities (provided by scheduler) for all generations.. but not yet preemption for all generations. But the most common use-case where we need this ends up being display composition (either fullscreen app/game or foreground app/game composited via overlay) so I haven't thought too much about the next step of boosting job priority. I might leave that to someone who already has preemption wired up ;-) BR, -R > I'm not sure whether you wire that one up as part of the conversion to > drm/sched. Without that I think we might need to ponder how we can do a > prio-boost for these, e.g. within a scheduling class we pick the jobs with > the nearest deadline first, before we pick others. > -Daniel > > > > --- > > > drivers/gpu/drm/scheduler/sched_fence.c | 25 +++++++++++++++++++++++++ > > > drivers/gpu/drm/scheduler/sched_main.c | 3 +++ > > > include/drm/gpu_scheduler.h | 6 ++++++ > > > 3 files changed, 34 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c > > > index 69de2c76731f..f389dca44185 100644 > > > --- a/drivers/gpu/drm/scheduler/sched_fence.c > > > +++ b/drivers/gpu/drm/scheduler/sched_fence.c > > > @@ -128,6 +128,30 @@ static void drm_sched_fence_release_finished(struct dma_fence *f) > > > dma_fence_put(&fence->scheduled); > > > } > > > +static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, > > > + ktime_t deadline) > > > +{ > > > + struct drm_sched_fence *fence = to_drm_sched_fence(f); > > > + unsigned long flags; > > > + > > > + spin_lock_irqsave(&fence->lock, flags); > > > + > > > + /* If we already have an earlier deadline, keep it: */ > > > + if (test_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && > > > + ktime_before(fence->deadline, deadline)) { > > > + spin_unlock_irqrestore(&fence->lock, flags); > > > + return; > > > + } > > > + > > > + fence->deadline = deadline; > > > + set_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags); > > > + > > > + spin_unlock_irqrestore(&fence->lock, flags); > > > + > > > + if (fence->parent) > > > + dma_fence_set_deadline(fence->parent, deadline); > > > +} > > > + > > > static const struct dma_fence_ops drm_sched_fence_ops_scheduled = { > > > .get_driver_name = drm_sched_fence_get_driver_name, > > > .get_timeline_name = drm_sched_fence_get_timeline_name, > > > @@ -138,6 +162,7 @@ static const struct dma_fence_ops drm_sched_fence_ops_finished = { > > > .get_driver_name = drm_sched_fence_get_driver_name, > > > .get_timeline_name = drm_sched_fence_get_timeline_name, > > > .release = drm_sched_fence_release_finished, > > > + .set_deadline = drm_sched_fence_set_deadline_finished, > > > }; > > > struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f) > > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c > > > index a2a953693b45..3ab0900d3596 100644 > > > --- a/drivers/gpu/drm/scheduler/sched_main.c > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c > > > @@ -818,6 +818,9 @@ static int drm_sched_main(void *param) > > > if (!IS_ERR_OR_NULL(fence)) { > > > s_fence->parent = dma_fence_get(fence); > > > + if (test_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT, > > > + &s_fence->finished.flags)) > > > + dma_fence_set_deadline(fence, s_fence->deadline); > > > > Maybe move this into a dma_sched_fence_set_parent() function. > > > > Apart from that looks good to me. > > > > Regards, > > Christian. > > > > > r = dma_fence_add_callback(fence, &sched_job->cb, > > > drm_sched_job_done_cb); > > > if (r == -ENOENT) > > > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h > > > index d18af49fd009..0f08ade614ae 100644 > > > --- a/include/drm/gpu_scheduler.h > > > +++ b/include/drm/gpu_scheduler.h > > > @@ -144,6 +144,12 @@ struct drm_sched_fence { > > > */ > > > struct dma_fence finished; > > > + /** > > > + * @deadline: deadline set on &drm_sched_fence.finished which > > > + * potentially needs to be propagated to &drm_sched_fence.parent > > > + */ > > > + ktime_t deadline; > > > + > > > /** > > > * @parent: the fence returned by &drm_sched_backend_ops.run_job > > > * when scheduling the job on hardware. We signal the > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Mon, Aug 16, 2021 at 03:25:20PM -0700, Rob Clark wrote: > On Mon, Aug 16, 2021 at 8:38 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > > > On Mon, Aug 16, 2021 at 12:14:35PM +0200, Christian König wrote: > > > Am 07.08.21 um 20:37 schrieb Rob Clark: > > > > From: Rob Clark <robdclark@chromium.org> > > > > > > > > As the finished fence is the one that is exposed to userspace, and > > > > therefore the one that other operations, like atomic update, would > > > > block on, we need to propagate the deadline from from the finished > > > > fence to the actual hw fence. > > > > > > > > Signed-off-by: Rob Clark <robdclark@chromium.org> > > > > I guess you're already letting the compositor run at a higher gpu priority > > so that your deadline'd drm_sched_job isn't stuck behind the app rendering > > the next frame? > > With the scheduler conversion we do have multiple priorities (provided > by scheduler) for all generations.. but not yet preemption for all > generations. > > But the most common use-case where we need this ends up being display > composition (either fullscreen app/game or foreground app/game > composited via overlay) so I haven't thought too much about the next > step of boosting job priority. I might leave that to someone who > already has preemption wired up ;-) Atm no-one, drm/sched isn't really aware that's a concept. I was more thinking of just boosting that request as a first step. Maybe within the same priority class we pick jobs with deadlines first, or something like that. Preempting is an entire can of worms on top. -Daniel > > BR, > -R > > > I'm not sure whether you wire that one up as part of the conversion to > > drm/sched. Without that I think we might need to ponder how we can do a > > prio-boost for these, e.g. within a scheduling class we pick the jobs with > > the nearest deadline first, before we pick others. > > -Daniel > > > > > > --- > > > > drivers/gpu/drm/scheduler/sched_fence.c | 25 +++++++++++++++++++++++++ > > > > drivers/gpu/drm/scheduler/sched_main.c | 3 +++ > > > > include/drm/gpu_scheduler.h | 6 ++++++ > > > > 3 files changed, 34 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c > > > > index 69de2c76731f..f389dca44185 100644 > > > > --- a/drivers/gpu/drm/scheduler/sched_fence.c > > > > +++ b/drivers/gpu/drm/scheduler/sched_fence.c > > > > @@ -128,6 +128,30 @@ static void drm_sched_fence_release_finished(struct dma_fence *f) > > > > dma_fence_put(&fence->scheduled); > > > > } > > > > +static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, > > > > + ktime_t deadline) > > > > +{ > > > > + struct drm_sched_fence *fence = to_drm_sched_fence(f); > > > > + unsigned long flags; > > > > + > > > > + spin_lock_irqsave(&fence->lock, flags); > > > > + > > > > + /* If we already have an earlier deadline, keep it: */ > > > > + if (test_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && > > > > + ktime_before(fence->deadline, deadline)) { > > > > + spin_unlock_irqrestore(&fence->lock, flags); > > > > + return; > > > > + } > > > > + > > > > + fence->deadline = deadline; > > > > + set_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags); > > > > + > > > > + spin_unlock_irqrestore(&fence->lock, flags); > > > > + > > > > + if (fence->parent) > > > > + dma_fence_set_deadline(fence->parent, deadline); > > > > +} > > > > + > > > > static const struct dma_fence_ops drm_sched_fence_ops_scheduled = { > > > > .get_driver_name = drm_sched_fence_get_driver_name, > > > > .get_timeline_name = drm_sched_fence_get_timeline_name, > > > > @@ -138,6 +162,7 @@ static const struct dma_fence_ops drm_sched_fence_ops_finished = { > > > > .get_driver_name = drm_sched_fence_get_driver_name, > > > > .get_timeline_name = drm_sched_fence_get_timeline_name, > > > > .release = drm_sched_fence_release_finished, > > > > + .set_deadline = drm_sched_fence_set_deadline_finished, > > > > }; > > > > struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f) > > > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c > > > > index a2a953693b45..3ab0900d3596 100644 > > > > --- a/drivers/gpu/drm/scheduler/sched_main.c > > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c > > > > @@ -818,6 +818,9 @@ static int drm_sched_main(void *param) > > > > if (!IS_ERR_OR_NULL(fence)) { > > > > s_fence->parent = dma_fence_get(fence); > > > > + if (test_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT, > > > > + &s_fence->finished.flags)) > > > > + dma_fence_set_deadline(fence, s_fence->deadline); > > > > > > Maybe move this into a dma_sched_fence_set_parent() function. > > > > > > Apart from that looks good to me. > > > > > > Regards, > > > Christian. > > > > > > > r = dma_fence_add_callback(fence, &sched_job->cb, > > > > drm_sched_job_done_cb); > > > > if (r == -ENOENT) > > > > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h > > > > index d18af49fd009..0f08ade614ae 100644 > > > > --- a/include/drm/gpu_scheduler.h > > > > +++ b/include/drm/gpu_scheduler.h > > > > @@ -144,6 +144,12 @@ struct drm_sched_fence { > > > > */ > > > > struct dma_fence finished; > > > > + /** > > > > + * @deadline: deadline set on &drm_sched_fence.finished which > > > > + * potentially needs to be propagated to &drm_sched_fence.parent > > > > + */ > > > > + ktime_t deadline; > > > > + > > > > /** > > > > * @parent: the fence returned by &drm_sched_backend_ops.run_job > > > > * when scheduling the job on hardware. We signal the > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c index 69de2c76731f..f389dca44185 100644 --- a/drivers/gpu/drm/scheduler/sched_fence.c +++ b/drivers/gpu/drm/scheduler/sched_fence.c @@ -128,6 +128,30 @@ static void drm_sched_fence_release_finished(struct dma_fence *f) dma_fence_put(&fence->scheduled); } +static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, + ktime_t deadline) +{ + struct drm_sched_fence *fence = to_drm_sched_fence(f); + unsigned long flags; + + spin_lock_irqsave(&fence->lock, flags); + + /* If we already have an earlier deadline, keep it: */ + if (test_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && + ktime_before(fence->deadline, deadline)) { + spin_unlock_irqrestore(&fence->lock, flags); + return; + } + + fence->deadline = deadline; + set_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags); + + spin_unlock_irqrestore(&fence->lock, flags); + + if (fence->parent) + dma_fence_set_deadline(fence->parent, deadline); +} + static const struct dma_fence_ops drm_sched_fence_ops_scheduled = { .get_driver_name = drm_sched_fence_get_driver_name, .get_timeline_name = drm_sched_fence_get_timeline_name, @@ -138,6 +162,7 @@ static const struct dma_fence_ops drm_sched_fence_ops_finished = { .get_driver_name = drm_sched_fence_get_driver_name, .get_timeline_name = drm_sched_fence_get_timeline_name, .release = drm_sched_fence_release_finished, + .set_deadline = drm_sched_fence_set_deadline_finished, }; struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f) diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index a2a953693b45..3ab0900d3596 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -818,6 +818,9 @@ static int drm_sched_main(void *param) if (!IS_ERR_OR_NULL(fence)) { s_fence->parent = dma_fence_get(fence); + if (test_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT, + &s_fence->finished.flags)) + dma_fence_set_deadline(fence, s_fence->deadline); r = dma_fence_add_callback(fence, &sched_job->cb, drm_sched_job_done_cb); if (r == -ENOENT) diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index d18af49fd009..0f08ade614ae 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -144,6 +144,12 @@ struct drm_sched_fence { */ struct dma_fence finished; + /** + * @deadline: deadline set on &drm_sched_fence.finished which + * potentially needs to be propagated to &drm_sched_fence.parent + */ + ktime_t deadline; + /** * @parent: the fence returned by &drm_sched_backend_ops.run_job * when scheduling the job on hardware. We signal the