[Intel-gfx] [PATCH v28 4/6] drm/i915: Add TGL+ SAGV support
Ville Syrjälä
ville.syrjala at linux.intel.com
Tue May 12 13:38:21 UTC 2020
On Tue, May 12, 2020 at 04:17:34PM +0300, Lisovskiy, Stanislav wrote:
> On Tue, May 12, 2020 at 04:10:46PM +0300, Ville Syrjälä wrote:
> > On Tue, May 12, 2020 at 03:52:27PM +0300, Lisovskiy, Stanislav wrote:
> > > On Tue, May 12, 2020 at 03:03:26PM +0300, Ville Syrjälä wrote:
> > > > On Thu, May 07, 2020 at 05:45:01PM +0300, Stanislav Lisovskiy wrote:
> > > > > Starting from TGL we need to have a separate wm0
> > > > > values for SAGV and non-SAGV which affects
> > > > > how calculations are done.
> > > > >
> > > > > v2: Remove long lines
> > > > > v3: Removed COLOR_PLANE enum references
> > > > > v4, v5, v6: Fixed rebase conflict
> > > > > v7: - Removed skl_plane_wm_level accessor from skl_allocate_pipe_ddb(Ville)
> > > > > - Removed sagv_uv_wm0(Ville)
> > > > > - can_sagv->use_sagv_wm(Ville)
> > > > >
> > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> > > > > ---
> > > > > drivers/gpu/drm/i915/display/intel_display.c | 8 +-
> > > > > .../drm/i915/display/intel_display_types.h | 2 +
> > > > > drivers/gpu/drm/i915/intel_pm.c | 118 +++++++++++++++++-
> > > > > 3 files changed, 121 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > index fd6d63b03489..be5741cb7595 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > @@ -13961,7 +13961,9 @@ static void verify_wm_state(struct intel_crtc *crtc,
> > > > > /* Watermarks */
> > > > > for (level = 0; level <= max_level; level++) {
> > > > > if (skl_wm_level_equals(&hw_plane_wm->wm[level],
> > > > > - &sw_plane_wm->wm[level]))
> > > > > + &sw_plane_wm->wm[level]) ||
> > > > > + (level == 0 && skl_wm_level_equals(&hw_plane_wm->wm[level],
> > > > > + &sw_plane_wm->sagv_wm0)))
> > > > > continue;
> > > > >
> > > > > drm_err(&dev_priv->drm,
> > > > > @@ -14016,7 +14018,9 @@ static void verify_wm_state(struct intel_crtc *crtc,
> > > > > /* Watermarks */
> > > > > for (level = 0; level <= max_level; level++) {
> > > > > if (skl_wm_level_equals(&hw_plane_wm->wm[level],
> > > > > - &sw_plane_wm->wm[level]))
> > > > > + &sw_plane_wm->wm[level]) ||
> > > > > + (level == 0 && skl_wm_level_equals(&hw_plane_wm->wm[level],
> > > > > + &sw_plane_wm->sagv_wm0)))
> > > > > continue;
> > > > >
> > > > > drm_err(&dev_priv->drm,
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > index 9488449e4b94..8cede29c9562 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > > @@ -688,11 +688,13 @@ struct skl_plane_wm {
> > > > > struct skl_wm_level wm[8];
> > > > > struct skl_wm_level uv_wm[8];
> > > > > struct skl_wm_level trans_wm;
> > > > > + struct skl_wm_level sagv_wm0;
> > > > > bool is_planar;
> > > > > };
> > > > >
> > > > > struct skl_pipe_wm {
> > > > > struct skl_plane_wm planes[I915_MAX_PLANES];
> > > > > + bool use_sagv_wm;
> > > > > };
> > > > >
> > > > > enum vlv_wm_level {
> > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > > index db188efee21e..934a686342ad 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > @@ -3863,25 +3863,35 @@ bool intel_can_enable_sagv(struct drm_i915_private *dev_priv,
> > > > > return bw_state->pipe_sagv_reject == 0;
> > > > > }
> > > > >
> > > > > +static bool
> > > > > +tgl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state);
> > > >
> > > > Just put the function here instead of adding fwd decalrations.
> > > >
> > > > > +
> > > > > static int intel_compute_sagv_mask(struct intel_atomic_state *state)
> > > > > {
> > > > > struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > > > > int ret;
> > > > > struct intel_crtc *crtc;
> > > > > - const struct intel_crtc_state *new_crtc_state;
> > > > > + struct intel_crtc_state *new_crtc_state;
> > > > > struct intel_bw_state *new_bw_state = NULL;
> > > > > const struct intel_bw_state *old_bw_state = NULL;
> > > > > int i;
> > > > >
> > > > > for_each_new_intel_crtc_in_state(state, crtc,
> > > > > new_crtc_state, i) {
> > > > > + bool can_sagv;
> > > > > +
> > > > > new_bw_state = intel_atomic_get_bw_state(state);
> > > > > if (IS_ERR(new_bw_state))
> > > > > return PTR_ERR(new_bw_state);
> > > > >
> > > > > old_bw_state = intel_atomic_get_old_bw_state(state);
> > > > >
> > > > > - if (skl_crtc_can_enable_sagv(new_crtc_state))
> > > > > + if (INTEL_GEN(dev_priv) >= 12)
> > > > > + can_sagv = tgl_crtc_can_enable_sagv(new_crtc_state);
> > > > > + else
> > > > > + can_sagv = skl_crtc_can_enable_sagv(new_crtc_state);
> > > > > +
> > > > > + if (can_sagv)
> > > > > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe);
> > > > > else
> > > > > new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe);
> > > > > @@ -3899,6 +3909,24 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state)
> > > > > return ret;
> > > > > }
> > > > >
> > > > > + for_each_new_intel_crtc_in_state(state, crtc,
> > > > > + new_crtc_state, i) {
> > > > > + struct skl_pipe_wm *pipe_wm = &new_crtc_state->wm.skl.optimal;
> > > > > +
> > > > > + /*
> > > > > + * Due to drm limitation at commit state, when
> > > > > + * changes are written the whole atomic state is
> > > > > + * zeroed away => which prevents from using it,
> > > > > + * so just sticking it into pipe wm state for
> > > > > + * keeping it simple - anyway this is related to wm.
> > > > > + * Proper way in ideal universe would be of course not
> > > > > + * to lose parent atomic state object from child crtc_state,
> > > > > + * and stick to OOP programming principles, which had been
> > > > > + * scientifically proven to work.
> > > > > + */
> > > >
> > > > More ramblings. Just drop this comment too imo.
> > >
> > > As I understand what is happening here is rather weird, so I thought
> > > commenting is good idea.
> >
> > At least I have no idea what the comment is trying to say.
> > I see nothing weird hapening here, we're just computing the
> > state which is totally standard stuff.
>
> Well I can remind, this is because there is no way to get parent state
> from crtc_state, because of weird drm atomic behaviour which leaves us
> with NULL parent state. So that we had to stick this boolean to some
> place.
> In normal code universe this wouldn't even be needed if we could
> just get atomic state from crtc_state when we write wm in skl_write_plane_wm.
Didn't get that at all from the comment. It talked about zeroing some
whole state which I took as 'memset(state, 0, sizeof(*state))'.
As that is not what's going on I just got confused by the comment.
Now that I understand what this is about I think the comment
(if we want to have one) should probably say something more like:
"we store use_sagv_wm in the crtc state rather than relying on
the bw state since we have no convenient way to get at the
latter from the plane commit hooks (especially in the legacy
cursor case)".
>
> However probably OOP principles like parent-child hieararchy is not a thing
> here. That what this comment was trying to say.
>
> In normal OOP you can't destroy parent object _before_ destroying
> child one.
There's no parent-child relationship between the crtc state and atomic
state (which really shouldn't be called a state to begin with, rather
it should be "transaction" or something along those lines).
--
Ville Syrjälä
Intel
More information about the Intel-gfx
mailing list