[Intel-gfx] [PATCH v21 05/10] drm/i915: Extract gen specific functions from intel_can_enable_sagv
Lisovskiy, Stanislav
stanislav.lisovskiy at intel.com
Wed Apr 8 15:54:09 UTC 2020
On Wed, Apr 08, 2020 at 05:55:02PM +0300, Ville Syrjälä wrote:
> On Wed, Apr 08, 2020 at 10:58:04AM +0300, Lisovskiy, Stanislav wrote:
> > On Tue, Apr 07, 2020 at 10:01:28PM +0300, Ville Syrjälä wrote:
> > > On Fri, Apr 03, 2020 at 09:20:03AM +0300, Stanislav Lisovskiy wrote:
> > > > Addressing one of the comments, recommending to extract platform
> > > > specific code from intel_can_enable_sagv as a preparation, before
> > > > we are going to add support for tgl+.
> > > >
> > > > Current code in intel_can_enable_sagv is valid only for skl,
> > > > so this patch adds also proper support for icl, subsequent
> > > > patches will add support for tgl+, combined with other required
> > > > changes.
> > > >
> > > > v2: - Renamed icl_can_enable_sagv into icl_crtc_can_enable_sagv(Ville)
> > > > - Removed dev variables(Ville)
> > > > - Constified crtc/plane_state in icl_crtc_can_enable_sagv
> > > > function(Ville)
> > > > - Added hw.active check(Ville)
> > > > - Refactored if ladder(Ville)
> > > >
> > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/intel_pm.c | 84 +++++++++++++++++++++------------
> > > > 1 file changed, 55 insertions(+), 29 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > index f8d62d1977ac..27d4d626cb34 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -3757,42 +3757,25 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
> > > > return 0;
> > > > }
> > > >
> > > > -bool intel_can_enable_sagv(struct intel_atomic_state *state)
> > > > +static bool icl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state)
> > > > {
> > > > - struct drm_device *dev = state->base.dev;
> > > > - struct drm_i915_private *dev_priv = to_i915(dev);
> > > > - struct intel_crtc *crtc;
> > > > + struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
> > > > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > > struct intel_plane *plane;
> > > > - struct intel_crtc_state *crtc_state;
> > > > - enum pipe pipe;
> > > > + const struct intel_plane_state *plane_state;
> > > > int level, latency;
> > > >
> > > > - if (!intel_has_sagv(dev_priv))
> > > > + if (crtc_state->hw.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE) {
> > > > + DRM_DEBUG_KMS("No SAGV for interlaced mode on pipe %c\n",
> > > > + pipe_name(crtc->pipe));
> > > > return false;
> > > > + }
> > > >
> > > > - /*
> > > > - * If there are no active CRTCs, no additional checks need be performed
> > > > - */
> > > > - if (hweight8(state->active_pipes) == 0)
> > > > + if (!crtc_state->hw.active)
> > >
> > > Should really be checked before anything else. Doesn't matter too much
> > > anymore since I made us clear the crtc state always, but still a bit
> > > inconsistent to look at other stuff in the state before we even know if
> > > the crtc is even enabled.
> > >
> > > > return true;
> > > >
> > > > - /*
> > > > - * SKL+ workaround: bspec recommends we disable SAGV when we have
> > > > - * more then one pipe enabled
> > > > - */
> > > > - if (hweight8(state->active_pipes) > 1)
> > > > - return false;
> > > > -
> > > > - /* Since we're now guaranteed to only have one active CRTC... */
> > > > - pipe = ffs(state->active_pipes) - 1;
> > > > - crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> > > > - crtc_state = to_intel_crtc_state(crtc->base.state);
> > > > -
> > > > - if (crtc_state->hw.adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
> > > > - return false;
> > > > -
> > > > - for_each_intel_plane_on_crtc(dev, crtc, plane) {
> > > > - struct skl_plane_wm *wm =
> > > > + intel_atomic_crtc_state_for_each_plane_state(plane, plane_state, crtc_state) {
> > > > + const struct skl_plane_wm *wm =
> > > > &crtc_state->wm.skl.optimal.planes[plane->id];
> > > >
> > > > /* Skip this plane if it's not enabled */
> > > > @@ -3807,7 +3790,7 @@ bool intel_can_enable_sagv(struct intel_atomic_state *state)
> > > > latency = dev_priv->wm.skl_latency[level];
> > > >
> > > > if (skl_needs_memory_bw_wa(dev_priv) &&
> > > > - plane->base.state->fb->modifier ==
> > > > + plane_state->uapi.fb->modifier ==
> > >
> > > hw.fb
> > >
> > > With those this is basically good, but still need to think how to avoid
> > > the regression due to only checking the crtcs in the state.
> >
> > Well tbh, initially you told me that this *_crtc_can_enable_sagv function extraction
> > can be "trivially" done as a separate patch :)) So I followed your instruction, and
> > then I got a comment saying that "this is now temporary busted because we are checking
> > only crtcs in a state". This kind of contraversial requirements - in order not to
> > have it "temporary busted", we should have introduced it at the same time with SAGV mask,
> > at the same time you wanted it to be extracted as a separate patch.
>
> TBF this patch does quite a bit more than extract the current code.
>
> What I think would work as a series is something like:
> patch 1:
> +intel_crtc_can_enable_sagv(crtc_state)
> {
> + stuff
> }
>
> intel_can_enable_sagv(state)
> {
> ...
> crtc_state = to_intel_crtc_state(crtc->base.state);
>
> - stuff
> + return intel_crtc_can_eanble_sagv(crtc_state);
> }
>
> patch 2:
> +sagv_pre_plane_update(state)
> +{
> + if (!intel_can_enable_sagv(state))
> + intel_disable_sagv(dev_priv);
> +}
>
> intel_atomic_commit_tail()
> {
> ...
> - if (!intel_can_enable_sagv(state))
> - intel_disable_sagv(dev_priv);
> + sagv_pre_plane_update(state);
> ...
> }
>
> (+ identical changes for post_plane_update())
>
> So far everything has been pure refactoring.
>
> patch 3:
> Introduce the sagv mask in bw state and precompute it using
> intel_crtc_can_enable_sagv() (while fixing the iterator issue therein),
> and update the pre/post hooks to consult said mask. Not quite pure
> refactoring anymore but seems like a bit more straightforward change
> now.
>
> At this point we should have a nicely precomputed sagv mask without
> intentional changes to current behaviour. After which it should be
> easier to extend this for new platforms.
This all makes sense, however in the end we'll have the same result as now, however this would
require to reshuffle the whole series...again.
Will try do it, the least painful way :)
>
> --
> Ville Syrjälä
> Intel
More information about the Intel-gfx
mailing list