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

["Das, Nirmoy" <nirmoy.das@linux.intel.com>]

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)