[Intel-xe] [PATCH] drm/i915/rps: split out display rps parts to a separate file
Rodrigo Vivi
rodrigo.vivi at intel.com
Wed Mar 22 20:57:17 UTC 2023
On Wed, Mar 22, 2023 at 04:27:54PM +0200, Jani Nikula wrote:
> 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?
I believe the easiest way will be to remove the display portion of the
initial mega-squash and create a display only patch/series and make that
separated.
Also starting to think to leave display out for the initial merge of Xe.
Then add display later.
>
> 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.
:) my bad!
> >
> > I still prefer it on <release-tag> or at the most drm-next.
well, in the end we will need to be on top of the drm-next. But meanwhile
we are flexible. I just wanted us to be on some point of a 6.2 with the
most of the display.
> 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