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@xxxxxxxxx> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > > --- > > 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@xxxxxxxxx> 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)