| Message ID | 20220901193228.255098-1-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915: Move some of the request decisions out of rps_boost function. | expand |
Hi Rodrigo, On 9/1/2022 9:32 PM, Rodrigo Vivi wrote: > Ideally all the decisions should be made before calling the boost function. > And rps functions only receiving the rps struct. At least lets move most > of the decisions to the request component, but still leave the test > and set of the fence flag boost inside the rps because that might be time > sensitive. > > Cc: Ashutosh Dixit <ashutosh.dixit@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 2 +- > drivers/gpu/drm/i915/gem/i915_gem_wait.c | 3 ++- > drivers/gpu/drm/i915/gt/intel_rps.c | 3 --- > drivers/gpu/drm/i915/gt/intel_rps.h | 1 + > drivers/gpu/drm/i915/i915_request.h | 5 +++-- > 5 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > index dd876dbbaa39..6967c47c7ba0 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > @@ -918,7 +918,7 @@ static int do_rps_boost(struct wait_queue_entry *_wait, > * is reasonable to assume that it will complete before the next > * vblank without our intervention, so leave RPS alone. > */ > - if (!i915_request_started(rq)) > + if (!i915_request_started(rq) && i915_request_needs_boost(rq)) > intel_rps_boost(rq); > i915_request_put(rq); > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c > index e6e01c2a74a6..2f2ca5e27248 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c > @@ -58,7 +58,8 @@ i915_gem_object_boost(struct dma_resv *resv, unsigned int flags) > dma_resv_usage_rw(flags & I915_WAIT_ALL)); > dma_resv_for_each_fence_unlocked(&cursor, fence) > if (dma_fence_is_i915(fence) && > - !i915_request_started(to_request(fence))) > + !i915_request_started(to_request(fence)) && > + i915_request_needs_boost(to_request(fence))) > intel_rps_boost(to_request(fence)); > dma_resv_iter_end(&cursor); > } > diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c > index 579ae9ac089c..2c8d9eeb7e7e 100644 > --- a/drivers/gpu/drm/i915/gt/intel_rps.c > +++ b/drivers/gpu/drm/i915/gt/intel_rps.c > @@ -1006,9 +1006,6 @@ void intel_rps_boost(struct i915_request *rq) > { > struct intel_guc_slpc *slpc; > > - if (i915_request_signaled(rq) || i915_request_has_waitboost(rq)) > - return; > - > /* Serializes with i915_request_retire() */ > if (!test_and_set_bit(I915_FENCE_FLAG_BOOST, &rq->fence.flags)) { > struct intel_rps *rps = &READ_ONCE(rq->engine)->gt->rps; > diff --git a/drivers/gpu/drm/i915/gt/intel_rps.h b/drivers/gpu/drm/i915/gt/intel_rps.h > index 4509dfdc52e0..9a053f1b04e8 100644 > --- a/drivers/gpu/drm/i915/gt/intel_rps.h > +++ b/drivers/gpu/drm/i915/gt/intel_rps.h > @@ -23,6 +23,7 @@ void intel_rps_disable(struct intel_rps *rps); > > void intel_rps_park(struct intel_rps *rps); > void intel_rps_unpark(struct intel_rps *rps); > +bool intel_rps_request_needs_boost(struct i915_request *rq); > void intel_rps_boost(struct i915_request *rq); > void intel_rps_dec_waiters(struct intel_rps *rps); > u32 intel_rps_get_boost_frequency(struct intel_rps *rps); > diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h > index 47041ec68df8..4f5049ef1ab9 100644 > --- a/drivers/gpu/drm/i915/i915_request.h > +++ b/drivers/gpu/drm/i915/i915_request.h > @@ -625,9 +625,10 @@ static inline void i915_request_mark_complete(struct i915_request *rq) > (u32 *)&rq->fence.seqno); > } > > -static inline bool i915_request_has_waitboost(const struct i915_request *rq) > +static inline bool i915_request_needs_boost(const struct i915_request *rq) > { > - return test_bit(I915_FENCE_FLAG_BOOST, &rq->fence.flags); > + return i915_request_signaled(rq) > + && test_bit(I915_FENCE_FLAG_BOOST, &rq->fence.flags); This could be i915_request_has_waitboost() or else AFAICS intel_rps_boost() is the only user of i915_request_has_waitboost() and that could be removed. Otherwise the series is: Acked-by: Nirmoy Das <nirmoy.das@intel.com> Nirmoy > } > > static inline bool i915_request_has_nopreempt(const struct i915_request *rq)
On Fri, 2022-09-02 at 11:07 +0200, Das, Nirmoy wrote: > Hi Rodrigo, > > On 9/1/2022 9:32 PM, Rodrigo Vivi wrote: > > Ideally all the decisions should be made before calling the boost > > function. > > And rps functions only receiving the rps struct. At least lets move > > most > > of the decisions to the request component, but still leave the test > > and set of the fence flag boost inside the rps because that might > > be time > > sensitive. > > > > Cc: Ashutosh Dixit <ashutosh.dixit@intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 2 +- > > drivers/gpu/drm/i915/gem/i915_gem_wait.c | 3 ++- > > drivers/gpu/drm/i915/gt/intel_rps.c | 3 --- > > drivers/gpu/drm/i915/gt/intel_rps.h | 1 + > > drivers/gpu/drm/i915/i915_request.h | 5 +++-- > > 5 files changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > index dd876dbbaa39..6967c47c7ba0 100644 > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > @@ -918,7 +918,7 @@ static int do_rps_boost(struct wait_queue_entry > > *_wait, > > * is reasonable to assume that it will complete before the > > next > > * vblank without our intervention, so leave RPS alone. > > */ > > - if (!i915_request_started(rq)) > > + if (!i915_request_started(rq) && > > i915_request_needs_boost(rq)) > > intel_rps_boost(rq); > > i915_request_put(rq); > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c > > b/drivers/gpu/drm/i915/gem/i915_gem_wait.c > > index e6e01c2a74a6..2f2ca5e27248 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c > > @@ -58,7 +58,8 @@ i915_gem_object_boost(struct dma_resv *resv, > > unsigned int flags) > > dma_resv_usage_rw(flags & > > I915_WAIT_ALL)); > > dma_resv_for_each_fence_unlocked(&cursor, fence) > > if (dma_fence_is_i915(fence) && > > - !i915_request_started(to_request(fence))) > > + !i915_request_started(to_request(fence)) && > > + i915_request_needs_boost(to_request(fence))) > > intel_rps_boost(to_request(fence)); > > dma_resv_iter_end(&cursor); > > } > > diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c > > b/drivers/gpu/drm/i915/gt/intel_rps.c > > index 579ae9ac089c..2c8d9eeb7e7e 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_rps.c > > +++ b/drivers/gpu/drm/i915/gt/intel_rps.c > > @@ -1006,9 +1006,6 @@ void intel_rps_boost(struct i915_request *rq) > > { > > struct intel_guc_slpc *slpc; > > > > - if (i915_request_signaled(rq) || > > i915_request_has_waitboost(rq)) > > - return; > > - > > /* Serializes with i915_request_retire() */ > > if (!test_and_set_bit(I915_FENCE_FLAG_BOOST, &rq- > > >fence.flags)) { > > struct intel_rps *rps = &READ_ONCE(rq->engine)->gt- > > >rps; > > diff --git a/drivers/gpu/drm/i915/gt/intel_rps.h > > b/drivers/gpu/drm/i915/gt/intel_rps.h > > index 4509dfdc52e0..9a053f1b04e8 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_rps.h > > +++ b/drivers/gpu/drm/i915/gt/intel_rps.h > > @@ -23,6 +23,7 @@ void intel_rps_disable(struct intel_rps *rps); > > > > void intel_rps_park(struct intel_rps *rps); > > void intel_rps_unpark(struct intel_rps *rps); > > +bool intel_rps_request_needs_boost(struct i915_request *rq); > > void intel_rps_boost(struct i915_request *rq); > > void intel_rps_dec_waiters(struct intel_rps *rps); > > u32 intel_rps_get_boost_frequency(struct intel_rps *rps); > > diff --git a/drivers/gpu/drm/i915/i915_request.h > > b/drivers/gpu/drm/i915/i915_request.h > > index 47041ec68df8..4f5049ef1ab9 100644 > > --- a/drivers/gpu/drm/i915/i915_request.h > > +++ b/drivers/gpu/drm/i915/i915_request.h > > @@ -625,9 +625,10 @@ static inline void > > i915_request_mark_complete(struct i915_request *rq) > > (u32 *)&rq->fence.seqno); > > } > > > > -static inline bool i915_request_has_waitboost(const struct > > i915_request *rq) > > +static inline bool i915_request_needs_boost(const struct > > i915_request *rq) > > { > > - return test_bit(I915_FENCE_FLAG_BOOST, &rq->fence.flags); > > + return i915_request_signaled(rq) > > + && test_bit(I915_FENCE_FLAG_BOOST, &rq- > > >fence.flags); > > This could be i915_request_has_waitboost() or else AFAICS > intel_rps_boost() is the only user of i915_request_has_waitboost() > > and that could be removed. > > Otherwise the series is: Acked-by: Nirmoy Das <nirmoy.das@intel.com> Thank you. I will actually hold this patch for now, because there's not much value alone and the next one is pending broader validation. I had resent the series with the only 2 simple patches that I want for now: https://patchwork.freedesktop.org/series/108075/ Thanks, Rodrigo. > > Nirmoy > > > } > > > > static inline bool i915_request_has_nopreempt(const struct > > i915_request *rq)
diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c index dd876dbbaa39..6967c47c7ba0 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c @@ -918,7 +918,7 @@ static int do_rps_boost(struct wait_queue_entry *_wait, * is reasonable to assume that it will complete before the next * vblank without our intervention, so leave RPS alone. */ - if (!i915_request_started(rq)) + if (!i915_request_started(rq) && i915_request_needs_boost(rq)) intel_rps_boost(rq); i915_request_put(rq); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c index e6e01c2a74a6..2f2ca5e27248 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c @@ -58,7 +58,8 @@ i915_gem_object_boost(struct dma_resv *resv, unsigned int flags) dma_resv_usage_rw(flags & I915_WAIT_ALL)); dma_resv_for_each_fence_unlocked(&cursor, fence) if (dma_fence_is_i915(fence) && - !i915_request_started(to_request(fence))) + !i915_request_started(to_request(fence)) && + i915_request_needs_boost(to_request(fence))) intel_rps_boost(to_request(fence)); dma_resv_iter_end(&cursor); } diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c index 579ae9ac089c..2c8d9eeb7e7e 100644 --- a/drivers/gpu/drm/i915/gt/intel_rps.c +++ b/drivers/gpu/drm/i915/gt/intel_rps.c @@ -1006,9 +1006,6 @@ void intel_rps_boost(struct i915_request *rq) { struct intel_guc_slpc *slpc; - if (i915_request_signaled(rq) || i915_request_has_waitboost(rq)) - return; - /* Serializes with i915_request_retire() */ if (!test_and_set_bit(I915_FENCE_FLAG_BOOST, &rq->fence.flags)) { struct intel_rps *rps = &READ_ONCE(rq->engine)->gt->rps; diff --git a/drivers/gpu/drm/i915/gt/intel_rps.h b/drivers/gpu/drm/i915/gt/intel_rps.h index 4509dfdc52e0..9a053f1b04e8 100644 --- a/drivers/gpu/drm/i915/gt/intel_rps.h +++ b/drivers/gpu/drm/i915/gt/intel_rps.h @@ -23,6 +23,7 @@ void intel_rps_disable(struct intel_rps *rps); void intel_rps_park(struct intel_rps *rps); void intel_rps_unpark(struct intel_rps *rps); +bool intel_rps_request_needs_boost(struct i915_request *rq); void intel_rps_boost(struct i915_request *rq); void intel_rps_dec_waiters(struct intel_rps *rps); u32 intel_rps_get_boost_frequency(struct intel_rps *rps); diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h index 47041ec68df8..4f5049ef1ab9 100644 --- a/drivers/gpu/drm/i915/i915_request.h +++ b/drivers/gpu/drm/i915/i915_request.h @@ -625,9 +625,10 @@ static inline void i915_request_mark_complete(struct i915_request *rq) (u32 *)&rq->fence.seqno); } -static inline bool i915_request_has_waitboost(const struct i915_request *rq) +static inline bool i915_request_needs_boost(const struct i915_request *rq) { - return test_bit(I915_FENCE_FLAG_BOOST, &rq->fence.flags); + return i915_request_signaled(rq) + && test_bit(I915_FENCE_FLAG_BOOST, &rq->fence.flags); } static inline bool i915_request_has_nopreempt(const struct i915_request *rq)
Ideally all the decisions should be made before calling the boost function. And rps functions only receiving the rps struct. At least lets move most of the decisions to the request component, but still leave the test and set of the fence flag boost inside the rps because that might be time sensitive. Cc: Ashutosh Dixit <ashutosh.dixit@intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/display/intel_atomic_plane.c | 2 +- drivers/gpu/drm/i915/gem/i915_gem_wait.c | 3 ++- drivers/gpu/drm/i915/gt/intel_rps.c | 3 --- drivers/gpu/drm/i915/gt/intel_rps.h | 1 + drivers/gpu/drm/i915/i915_request.h | 5 +++-- 5 files changed, 7 insertions(+), 7 deletions(-)