[Intel-xe] [PATCH] drm/i915/rps: split out display rps parts to a separate file
Jani Nikula
jani.nikula at intel.com
Wed Mar 22 09:15:05 UTC 2023
On Tue, 21 Mar 2023, Lucas De Marchi <lucas.demarchi at intel.com> wrote:
> On Tue, Mar 21, 2023 at 06:08:39PM +0200, Jani Nikula wrote:
>>On Tue, 21 Mar 2023, Jani Nikula <jani.nikula at intel.com> wrote:
>>> On Tue, 21 Mar 2023, Jani Nikula <jani.nikula at intel.com> wrote:
>>>> Split out the RPS parts so they can be conditionally compiled out later.
>>>>
>>>> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
>>>> Reviewed-by: Ville Syrjälä <ville.syrjala at 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)
Even if that results in a broken build for a commit?
I also have i915 patches locally that I think are probably fine upstream
*with* the xe submission, but not standalone. They clean up #ifdefs. The
trouble is, they often break the build on top of drm-xe-next, unless I
also fix the commit adding #ifdefs. And in a rebase, they should go
*before* 38a139546384.
Here's a good example: https://paste.debian.net/1274882/
That commit should go before 38a139546384, with -DI915=1 added to i915
build, but it also touches xe to keep building properly.
Here, I admit a git-pile setup would help this part of the job.
> 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:
I thought the rebase was going to be on v6.2, I argued for basing on
drm-intel-next, but it ended up being on drm-tip after all. I was kind
of surprised.
> 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.
If only there was a way to tell git rebase how to reorder the
patches. That would be needed for the ones that aren't upstream.
BR,
Jani.
>
> 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
--
Jani Nikula, Intel Open Source Graphics Center
More information about the Intel-xe
mailing list