Re: [Intel-xe] [PATCH] drm/i915/rps: split out display rps parts to a separate file

[Lucas De Marchi <lucas.demarchi@intel.com>]

On Tue, Mar 21, 2023 at 06:08:39PM +0200, Jani Nikula wrote:
>On Tue, 21 Mar 2023, Jani Nikula <jani.nikula@intel.com> wrote:
>> On Tue, 21 Mar 2023, Jani Nikula <jani.nikula@intel.com> wrote:
>>> Split out the RPS parts so they can be conditionally compiled out later.
>>>
>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Link: https://patchwork.freedesktop.org/patch/msgid/20230302164936.3034161-1-jani.nikula@intel.com
>>> (cherry picked from commit 6dbbff25b39565c801c87379bc85933fb436518e)
>>>
>>> ---
>>>
>>> Here's an example of an upstream i915 commit that can help clean up
>>> cherry-pick as well as cleanup of the #ifdefs in one go, otherwise it
>>> breaks bisect.
>>
>> *sigh* I had something more here, but having #ifdef first on a line
>> dropped it as comments, and turned it into nonsense.
>>
>> It's a backport of upstream i915 commit to help clean up #ifdefs, but
>> how do we want to handle them?
>
>Moreover, even if we're stalling with adding such #ifdefs to upstream
>i915 now, we can however clean up existing usage in the xe branch to be
>more suitable for upstream. For example, I'd take the #ifdefs here
>pretty much without issues.

instead of a cherry-pick + squashed-ifdef-removal I think it's better
to do:

1) cherry-pick
2) remove ifdefs and commit with

	git commit -s --fixup=38a139546384

   ... or whatever commit added them (there are a few before that commit
   above)

The reason is that I don't think we will be able to avoid 1 more rebase
before being merged upstream.  We can't do merge-approach neither
because of these commits we don't really want to ever show up in
upstream (and because we are on top of drm-tip rather than e.g.
drm-next). So doing the above at least makes a rebase more bearable:

Whenever we do a rebase (with --autosquash) the cherry-pick should
automatically be dropped by git and the fixup should automatically remove
the addition of the ifdefs in the commit that added them.

Lucas De Marchi

>
>BR,
>Jani.
>
>
>>
>>
>>> ---
>>>  drivers/gpu/drm/i915/Makefile                 |  1 +
>>>  .../gpu/drm/i915/display/intel_atomic_plane.c | 85 ++-----------------
>>>  .../gpu/drm/i915/display/intel_display_rps.c  | 81 ++++++++++++++++++
>>>  .../gpu/drm/i915/display/intel_display_rps.h  | 34 ++++++++
>>>  4 files changed, 123 insertions(+), 78 deletions(-)
>>>  create mode 100644 drivers/gpu/drm/i915/display/intel_display_rps.c
>>>  create mode 100644 drivers/gpu/drm/i915/display/intel_display_rps.h
>>>
>>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>>> index ae9259ab3261..f689132e1506 100644
>>> --- a/drivers/gpu/drm/i915/Makefile
>>> +++ b/drivers/gpu/drm/i915/Makefile
>>> @@ -239,6 +239,7 @@ i915-y += \
>>>  	display/intel_display_power.o \
>>>  	display/intel_display_power_map.o \
>>>  	display/intel_display_power_well.o \
>>> +	display/intel_display_rps.o \
>>>  	display/intel_dmc.o \
>>>  	display/intel_dpio_phy.o \
>>>  	display/intel_dpll.o \
>>> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
>>> index 3d37f44ec835..e666a5c1fd91 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
>>> @@ -34,13 +34,10 @@
>>>  #include <drm/drm_atomic_helper.h>
>>>  #include <drm/drm_fourcc.h>
>>>
>>> -#ifdef I915
>>> -#include "gt/intel_rps.h"
>>> -#endif
>>> -
>>>  #include "i915_config.h"
>>>  #include "intel_atomic_plane.h"
>>>  #include "intel_cdclk.h"
>>> +#include "intel_display_rps.h"
>>>  #include "intel_display_trace.h"
>>>  #include "intel_display_types.h"
>>>  #include "intel_fb.h"
>>> @@ -951,66 +948,6 @@ int intel_atomic_plane_check_clipping(struct intel_plane_state *plane_state,
>>>  	return 0;
>>>  }
>>>
>>> -#ifdef I915
>>> -struct wait_rps_boost {
>>> -	struct wait_queue_entry wait;
>>> -
>>> -	struct drm_crtc *crtc;
>>> -	struct i915_request *request;
>>> -};
>>> -
>>> -static int do_rps_boost(struct wait_queue_entry *_wait,
>>> -			unsigned mode, int sync, void *key)
>>> -{
>>> -	struct wait_rps_boost *wait = container_of(_wait, typeof(*wait), wait);
>>> -	struct i915_request *rq = wait->request;
>>> -
>>> -	/*
>>> -	 * If we missed the vblank, but the request is already running it
>>> -	 * is reasonable to assume that it will complete before the next
>>> -	 * vblank without our intervention, so leave RPS alone.
>>> -	 */
>>> -	if (!i915_request_started(rq))
>>> -		intel_rps_boost(rq);
>>> -	i915_request_put(rq);
>>> -
>>> -	drm_crtc_vblank_put(wait->crtc);
>>> -
>>> -	list_del(&wait->wait.entry);
>>> -	kfree(wait);
>>> -	return 1;
>>> -}
>>> -
>>> -static void add_rps_boost_after_vblank(struct drm_crtc *crtc,
>>> -				       struct dma_fence *fence)
>>> -{
>>> -	struct wait_rps_boost *wait;
>>> -
>>> -	if (!dma_fence_is_i915(fence))
>>> -		return;
>>> -
>>> -	if (DISPLAY_VER(to_i915(crtc->dev)) < 6)
>>> -		return;
>>> -
>>> -	if (drm_crtc_vblank_get(crtc))
>>> -		return;
>>> -
>>> -	wait = kmalloc(sizeof(*wait), GFP_KERNEL);
>>> -	if (!wait) {
>>> -		drm_crtc_vblank_put(crtc);
>>> -		return;
>>> -	}
>>> -
>>> -	wait->request = to_request(dma_fence_get(fence));
>>> -	wait->crtc = crtc;
>>> -
>>> -	wait->wait.func = do_rps_boost;
>>> -	wait->wait.flags = 0;
>>> -
>>> -	add_wait_queue(drm_crtc_vblank_waitqueue(crtc), &wait->wait);
>>> -}
>>> -#endif
>>> -
>>>  /**
>>>   * intel_prepare_plane_fb - Prepare fb for usage on plane
>>>   * @_plane: drm plane to prepare for
>>> @@ -1102,13 +1039,13 @@ intel_prepare_plane_fb(struct drm_plane *_plane,
>>>  		dma_resv_iter_begin(&cursor, obj->base.resv,
>>>  				    DMA_RESV_USAGE_WRITE);
>>>  		dma_resv_for_each_fence_unlocked(&cursor, fence) {
>>> -			add_rps_boost_after_vblank(new_plane_state->hw.crtc,
>>> -						   fence);
>>> +			intel_display_rps_boost_after_vblank(new_plane_state->hw.crtc,
>>> +							     fence);
>>>  		}
>>>  		dma_resv_iter_end(&cursor);
>>>  	} else {
>>> -		add_rps_boost_after_vblank(new_plane_state->hw.crtc,
>>> -					   new_plane_state->uapi.fence);
>>> +		intel_display_rps_boost_after_vblank(new_plane_state->hw.crtc,
>>> +						     new_plane_state->uapi.fence);
>>>  	}
>>>
>>>  	/*
>>> @@ -1119,10 +1056,7 @@ intel_prepare_plane_fb(struct drm_plane *_plane,
>>>  	 * that are not quite steady state without resorting to forcing
>>>  	 * maximum clocks following a vblank miss (see do_rps_boost()).
>>>  	 */
>>> -	if (!state->rps_interactive) {
>>> -		intel_rps_mark_interactive(&to_gt(dev_priv)->rps, true);
>>> -		state->rps_interactive = true;
>>> -	}
>>> +	intel_display_rps_mark_interactive(dev_priv, state, true);
>>>
>>>  	return 0;
>>>
>>> @@ -1159,12 +1093,7 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>>>  	if (!obj)
>>>  		return;
>>>
>>> -#ifdef I915
>>> -	if (state->rps_interactive) {
>>> -		intel_rps_mark_interactive(&to_gt(dev_priv)->rps, false);
>>> -		state->rps_interactive = false;
>>> -	}
>>> -#endif
>>> +	intel_display_rps_mark_interactive(dev_priv, state, false);
>>>
>>>  	/* Should only be called after a successful intel_prepare_plane_fb()! */
>>>  	intel_plane_unpin_fb(old_plane_state);
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_rps.c b/drivers/gpu/drm/i915/display/intel_display_rps.c
>>> new file mode 100644
>>> index 000000000000..918d0327169a
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/i915/display/intel_display_rps.c
>>> @@ -0,0 +1,81 @@
>>> +// SPDX-License-Identifier: MIT
>>> +/*
>>> + * Copyright © 2023 Intel Corporation
>>> + */
>>> +
>>> +#include <drm/drm_crtc.h>
>>> +#include <drm/drm_vblank.h>
>>> +
>>> +#include "gt/intel_rps.h"
>>> +#include "i915_drv.h"
>>> +#include "intel_display_rps.h"
>>> +#include "intel_display_types.h"
>>> +
>>> +struct wait_rps_boost {
>>> +	struct wait_queue_entry wait;
>>> +
>>> +	struct drm_crtc *crtc;
>>> +	struct i915_request *request;
>>> +};
>>> +
>>> +static int do_rps_boost(struct wait_queue_entry *_wait,
>>> +			unsigned mode, int sync, void *key)
>>> +{
>>> +	struct wait_rps_boost *wait = container_of(_wait, typeof(*wait), wait);
>>> +	struct i915_request *rq = wait->request;
>>> +
>>> +	/*
>>> +	 * If we missed the vblank, but the request is already running it
>>> +	 * is reasonable to assume that it will complete before the next
>>> +	 * vblank without our intervention, so leave RPS alone.
>>> +	 */
>>> +	if (!i915_request_started(rq))
>>> +		intel_rps_boost(rq);
>>> +	i915_request_put(rq);
>>> +
>>> +	drm_crtc_vblank_put(wait->crtc);
>>> +
>>> +	list_del(&wait->wait.entry);
>>> +	kfree(wait);
>>> +	return 1;
>>> +}
>>> +
>>> +void intel_display_rps_boost_after_vblank(struct drm_crtc *crtc,
>>> +					  struct dma_fence *fence)
>>> +{
>>> +	struct wait_rps_boost *wait;
>>> +
>>> +	if (!dma_fence_is_i915(fence))
>>> +		return;
>>> +
>>> +	if (DISPLAY_VER(to_i915(crtc->dev)) < 6)
>>> +		return;
>>> +
>>> +	if (drm_crtc_vblank_get(crtc))
>>> +		return;
>>> +
>>> +	wait = kmalloc(sizeof(*wait), GFP_KERNEL);
>>> +	if (!wait) {
>>> +		drm_crtc_vblank_put(crtc);
>>> +		return;
>>> +	}
>>> +
>>> +	wait->request = to_request(dma_fence_get(fence));
>>> +	wait->crtc = crtc;
>>> +
>>> +	wait->wait.func = do_rps_boost;
>>> +	wait->wait.flags = 0;
>>> +
>>> +	add_wait_queue(drm_crtc_vblank_waitqueue(crtc), &wait->wait);
>>> +}
>>> +
>>> +void intel_display_rps_mark_interactive(struct drm_i915_private *i915,
>>> +					struct intel_atomic_state *state,
>>> +					bool interactive)
>>> +{
>>> +	if (state->rps_interactive == interactive)
>>> +		return;
>>> +
>>> +	intel_rps_mark_interactive(&to_gt(i915)->rps, interactive);
>>> +	state->rps_interactive = interactive;
>>> +}
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_rps.h b/drivers/gpu/drm/i915/display/intel_display_rps.h
>>> new file mode 100644
>>> index 000000000000..2cfb8a303b77
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/i915/display/intel_display_rps.h
>>> @@ -0,0 +1,34 @@
>>> +/* SPDX-License-Identifier: MIT */
>>> +/*
>>> + * Copyright © 2023 Intel Corporation
>>> + */
>>> +
>>> +#ifndef __INTEL_DISPLAY_RPS_H__
>>> +#define __INTEL_DISPLAY_RPS_H__
>>> +
>>> +#include <linux/types.h>
>>> +
>>> +struct dma_fence;
>>> +struct drm_crtc;
>>> +struct drm_i915_private;
>>> +struct intel_atomic_state;
>>> +
>>> +#ifdef I915
>>> +void intel_display_rps_boost_after_vblank(struct drm_crtc *crtc,
>>> +					  struct dma_fence *fence);
>>> +void intel_display_rps_mark_interactive(struct drm_i915_private *i915,
>>> +					struct intel_atomic_state *state,
>>> +					bool interactive);
>>> +#else
>>> +static inline void intel_display_rps_boost_after_vblank(struct drm_crtc *crtc,
>>> +							struct dma_fence *fence)
>>> +{
>>> +}
>>> +static inline void intel_display_rps_mark_interactive(struct drm_i915_private *i915,
>>> +						      struct intel_atomic_state *state,
>>> +						      bool interactive)
>>> +{
>>> +}
>>> +#endif
>>> +
>>> +#endif /* __INTEL_DISPLAY_RPS_H__ */
>
>-- 
>Jani Nikula, Intel Open Source Graphics Center