Re: [Intel-gfx] [PATCH 1/2] drm/i915: Move some of the request decisions out of rps_boost function.

["Vivi, Rodrigo" <rodrigo.vivi@intel.com>]

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)