[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 14:27:54 UTC 2023
On Wed, 22 Mar 2023, Lucas De Marchi <lucas.demarchi at intel.com> wrote:
> On Wed, Mar 22, 2023 at 11:15:05AM +0200, Jani Nikula wrote:
>>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?
>
> yes, I guess we don't have many options. Better stick to the one that
> can produce a clean result at the end
>
>>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.
>
> I guess the option is to state that in the commit message.
> Maybe hacking a git command
>
> git commit -a --fixup=38a139546384^
>
> and then make sure the commit says something about NOFIXUP. So whoever
> is rebasing knows this is about a commit move only. I can type a script
> to change the rebase.todo automatically to do that if needed.
> Maybe git would be open to accept the logic for a `git commit
> --fixup=[move-after|move-before]:`
Even that doesn't work nicely, because there's so much cruft already
that we can't have clean i915 commits on top of drm-xe-next.
The whole file movement to compat-i915-headers is also still part of the
history.
When do we start squashing those away?
BR,
Jani.
>>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.
>
> I still prefer it on <release-tag> or at the most drm-next. But does it
> matter much if we still keep these commits that can't go as-is upstream?
> If we didn't have them we could simply have a merge approach and be on
> a <release-tag>, which would be much simpler.
>
> Lucas De Marchi
>
>>
>>> 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
--
Jani Nikula, Intel Open Source Graphics Center
More information about the Intel-xe
mailing list