[Intel-xe] [PATCH] drm/i915/rps: split out display rps parts to a separate file
Lucas De Marchi
lucas.demarchi at intel.com
Wed Mar 22 13:28:41 UTC 2023
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]:`
>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
More information about the Intel-xe
mailing list