On Mon, Oct 14, 2019 at 04:13:31AM -0700, Lisovskiy, Stanislav wrote:
> On Fri, 2019-10-11 at 16:49 -0700, James Ausmus wrote:
> > On Wed, Sep 25, 2019 at 03:17:37PM +0300, Stanislav Lisovskiy wrote:
> > > According to BSpec 53998, we should try to
> > > restrict qgv points, which can't provide
> > > enough bandwidth for desired display configuration.
> > >
> > > Currently we are just comparing against all of
> > > those and take minimum(worst case).
> > >
> > > v2: Fixed wrong PCode reply mask, removed hardcoded
> > > values.
> > >
> > > v3: Forbid simultaneous legacy SAGV PCode requests and
> > > restricting qgv points. Put the actual restriction
> > > to commit function, added serialization(thanks to Ville)
> > > to prevent commit being applied out of order in case of
> > > nonblocking and/or nomodeset commits.
>
> Hi James,
>
> Thank you for great review!
>
> While many of your comments are definitely
> good findings, still will leave reply to a few,
> just to keep things clear.
>
>
> > >
> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxx>
> > > Cc: James Ausmus <james.ausmus@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_atomic.c | 16 ++++
> > > drivers/gpu/drm/i915/display/intel_atomic.h | 3 +
> > > drivers/gpu/drm/i915/display/intel_bw.c | 79 +++++++++++++
> > > ------
> > > drivers/gpu/drm/i915/display/intel_bw.h | 2 +
> > > drivers/gpu/drm/i915/display/intel_display.c | 78
> > > +++++++++++++++++-
> > > .../drm/i915/display/intel_display_types.h | 3 +
> > > drivers/gpu/drm/i915/i915_drv.h | 2 +
> > > drivers/gpu/drm/i915/i915_reg.h | 3 +
> > > 8 files changed, 160 insertions(+), 26 deletions(-)
>
> > if (max_data_rate >= data_rate)
> > allowed_points |= 1 << i;
> > DRM_DEBUG_KMS...
> >
> > > + allowed_points |= 1 << i;
> > > + }
> >
> > According to the BSpec page, we also need to save off the QGV point
> > that has
> > the most available bandwidth:
> >
> > "At least one GV point must always remain unmasked. The point
> > providing the
> > highest bandwidth for display must always remain unmasked."
> >
> > We should stash that point separately, and ensure it always remains
> > unmasked.
> >
> > > + }
> > > +
> > > + if (allowed_points == 0) {
> > > + DRM_DEBUG_KMS("Could not find any suitable QGV
> > > points\n");
> > > return -EINVAL;
> > > }
>
> This actually guarantees that, I think - we will never allow a
> config which will require us to mask all of the points to work.
Yeah, fair enough - if *any* points allow a high enough bandwidth, then
it's certain that the *highest* BW point will be unmasked. If *no*
points allow enough BW, then it doesn't matter anyway because we're
going to fail out and not allow that config...
Thanks!
-James
>
> > >
> > > + state->qgv_points_mask = (~allowed_points) & ((1 <<
> > > qi.num_points) - 1);
> > > +
> > > + /*
> > > + * If the actual mask had changed we need to make sure that
> > > + * the commits are serialized(in case this is a nomodeset,
> > > nonblocking)
> > > + */
> > > + if (state->qgv_points_mask != dev_priv->qgv_points_mask) {
> > > + ret = intel_atomic_serialize_global_state(state);
> > > + if (ret) {
> > > + DRM_DEBUG_KMS("Could not serialize global
> > > state\n");
> > > + return ret;
> > > + }
> > > + }
> > > +
> > > return 0;
> > > }
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h
> > > b/drivers/gpu/drm/i915/display/intel_bw.h
> > > index 9db10af012f4..66bf9bc10b73 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_bw.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_bw.h
> > > @@ -28,5 +28,7 @@ int intel_bw_init(struct drm_i915_private
> > > *dev_priv);
> > > int intel_bw_atomic_check(struct intel_atomic_state *state);
> > > void intel_bw_crtc_update(struct intel_bw_state *bw_state,
> > > const struct intel_crtc_state *crtc_state);
> > > +int icl_pcode_restrict_qgv_points(struct drm_i915_private
> > > *dev_priv,
> > > + u32 points_mask);
> > >
> > > #endif /* __INTEL_BW_H__ */
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 5ecf54270181..c3196d0e4be3 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -13960,6 +13960,68 @@ static void
> > > intel_atomic_cleanup_work(struct work_struct *work)
> > > intel_atomic_helper_free_state(i915);
> > > }
> > >
> > > +static void intel_qgv_point_pre_update(struct intel_atomic_state
> > > *state)
> >
> > It would be nice to name this either "mask" or "unmask", so it's
> > easier
> > at first glance to see which function is turning on masked bits vs
> > toggling them off.
> >
> > > +{
> > > + struct drm_device *dev = state->base.dev;
> > > + struct drm_i915_private *dev_priv = to_i915(dev);
> > > + int i, ret;
> > > +
> > > + /*
> > > + * Restrict required qgv points before updating the
> > > configuration.
> > > + * According to BPsec we can't mask and unmask qgv points at
> > > the same
> >
> > s/BPsec/BSpec/
> >
> > > + * time. Also masking should be done before updating the
> > > configuration
> > > + * and unmasking afterwards.
> > > + */
> > > + u32 new_qgv_points_mask = dev_priv->qgv_points_mask;
> > > + int num_points = dev_priv->max_bw[0].num_qgv_points;
> > > +
> > > + for (i = num_points; i > 0; i--) {
> > > + int new_mask_bit = state->qgv_points_mask & (1 <<
> > > num_points);
> > > + int old_mask_bit = new_qgv_points_mask & (1 <<
> > > num_points);
> >
> > The naming here is spinning my head a bit, as we're getting the
> > "old_mask_bit" from the "new_qgv_points_mask".
> >
> > > +
> > > + if (old_mask_bit != new_mask_bit)
> > > + if (new_mask_bit != 0)
> >
> > Can't this just be
> >
> > if (new_mask_bit)
> > new_qgv_points_mask |= new_mask_bit;
> >
> > ?
> >
> > Since the only way that (old_mask_bit != new_mask_bit) is when we're
> > going from a 0 to a 1, and it's ok to go from a 1 to a 1, so the only
> > thing that matters here is new_mask_bit, right? If that's the case,
> > can't you just drop the old_mask_bit parts entirely? Or am I
> > confusing
> > myself with the naming? :)
> >
> >
> > Actually, wouldn't the whole function at that point just be:
> >
> > ret = icl_pcode_restrict_qgv_points(dev_priv, dev_priv-
> > >qgv_points_mask | state->qgv_points_mask);
>
> Sure it could be, however for some reason I thought that you can
> mask/unmask only one point per request. However now can't find that in
> BSpec - just states that you can only mask or unmask points(was it my
> mistake or did somebody edit it?), but not both, so if I won't find any
> contradiction here, that is definitely way to go :)
>
> >
> >
> > Since you're just wanting to "turn on" masking points in the _pre,
> > and
> > leave the "turn off" of masking points to the _post?
> >
> > > + new_qgv_points_mask |= new_mask_bit;
> > > + }
> > > +
> > > + ret = icl_pcode_restrict_qgv_points(dev_priv,
> > > new_qgv_points_mask);
> > > + if (ret < 0)
> > > + DRM_DEBUG_KMS("Could not restrict required gqv
> > > points(%d)\n", ret);
> >
> > s/gqv/qgv/
> >
> >
> > Also, if we fail masking off the qgv points that can't support our BW
> > req, shouldn't we handle that failure somehow - maybe just disable
> > SAGV
> > entirely? Better we lose power than have flickering screens...
>
> Sounds reasonable, need to discuss that with Ville. However I would
> may be still stick to simply rejecting that config and assume that
> at least currently we are ok, otherwise if we can't even succeed with
> sending a PCode request to restrict points, means that something is so
> weird that we might not succeed with disabling it as well.
>
> >
> > > + else
> > > + dev_priv->qgv_points_mask = new_qgv_points_mask;
> > > +}
> > > +
> > > +static void intel_qgv_point_post_update(struct intel_atomic_state
> > > *state)
> >
> > Same comment on the naming
> >
> > > +{
> > > + struct drm_device *dev = state->base.dev;
> > > + struct drm_i915_private *dev_priv = to_i915(dev);
> > > + int i, ret;
> > > +
> > > + /*
> > > + * Restrict required qgv points before updating the
> > > configuration.
> > > + * According to BPsec we can't mask and unmask qgv points at
> > > the same
> >
> > s/BPsec/BSpec/
> >
> > > + * time. Also masking should be done before updating the
> > > configuration
> > > + * and unmasking afterwards.
> > > + */
> > > + u32 new_qgv_points_mask = dev_priv->qgv_points_mask;
> > > + int num_points = dev_priv->max_bw[0].num_qgv_points;
> > > +
> > > + for (i = num_points; i > 0; i--) {
> > > + int new_mask_bit = state->qgv_points_mask & (1 <<
> > > num_points);
> > > + int old_mask_bit = new_qgv_points_mask & (1 <<
> > > num_points);
> > > +
> > > + if (old_mask_bit != new_mask_bit)
> > > + if (new_mask_bit == 0)
> > > + new_qgv_points_mask &= ~old_mask_bit;
> > > + }
> >
> > Same comment here - can't this really be simplified to:
> >
> > ret = icl_pcode_restrict_qgv_points(dev_priv, dev_priv-
> > >qgv_points_mask & state->qgv_points_mask);
> >
> > Since here we're just wnating to "turn off" the mask for points that
> > the
> > new state allows, and we should have already "turned on" all the
> > points
> > in _pre?
> >
> > > +
> > > + ret = icl_pcode_restrict_qgv_points(dev_priv,
> > > new_qgv_points_mask);
> > > + if (ret < 0)
> > > + DRM_DEBUG_KMS("Could not restrict required gqv
> > > points(%d)\n", ret);
> >
> > Maybe change the error message to something like "Could not enable
> > required qgv points", so it's more easily differentiated?
> >
> > Same here about error handling - if we fail to enable qgv points that
> > may be required, we might just want to entirely disable SAGV, as we
> > might not have a point that works for our BW reqs, and it's better to
> > lose power than flicker.
>
> In addition to what I said above I also know remembered a concern
> regarding that we probably shouldn't combine intel_enable/disable_sagv
> and this new Pcode request, so for sake of simplicity may be just
> reject that config? We really need to discuss this.
>
> >
> > > + else
> > > + dev_priv->qgv_points_mask = new_qgv_points_mask;
> > > +}
> > > +
> > > static void intel_atomic_commit_tail(struct intel_atomic_state
> > > *state)
> > > {
> > > struct drm_device *dev = state->base.dev;
> > > @@ -13987,6 +14049,9 @@ static void intel_atomic_commit_tail(struct
> > > intel_atomic_state *state)
> > > }
> > > }
> > >
> > > + if ((INTEL_GEN(dev_priv) >= 11))
> > > + intel_qgv_point_pre_update(state);
> > > +
> > > intel_commit_modeset_disables(state);
> > >
> > > /* FIXME: Eventually get rid of our crtc->config pointer */
> > > @@ -14005,8 +14070,9 @@ static void intel_atomic_commit_tail(struct
> > > intel_atomic_state *state)
> > > * SKL workaround: bspec recommends we disable the SAGV
> > > when we
> > > * have more then one pipe enabled
> > > */
> > > - if (!intel_can_enable_sagv(state))
> > > - intel_disable_sagv(dev_priv);
> > > + if (INTEL_GEN(dev_priv) < 11)
> > > + if (!intel_can_enable_sagv(state))
> > > + intel_disable_sagv(dev_priv);
> > >
> > > intel_modeset_verify_disabled(dev_priv, state);
> > > }
> > > @@ -14084,8 +14150,12 @@ static void
> > > intel_atomic_commit_tail(struct intel_atomic_state *state)
> > > if (state->modeset)
> > > intel_verify_planes(state);
> > >
> > > - if (state->modeset && intel_can_enable_sagv(state))
> > > - intel_enable_sagv(dev_priv);
> > > + if (INTEL_GEN(dev_priv) < 11)
> > > + if (state->modeset && intel_can_enable_sagv(state))
> > > + intel_enable_sagv(dev_priv);
> > > +
> > > + if ((INTEL_GEN(dev_priv) >= 11) &&
> > > intel_can_enable_sagv(state))
> > > + intel_qgv_point_post_update(state);
> >
> > I keep going back and forth in my mind about the above block - what
> > do you
> > think of doing it this way?
> >
> > if (intel_can_enable_sagv(state)) {
> > if (INTEL_GEN(dev_priv) >= 11)
> > intel_qgv_point_post_update(state);
> > else if (state->modeset)
> > intel_enable_sagv(dev_priv);
> > }
> >
> >
> > Feels a little cleaner, I think, and lets us keep our standard New
> > Gen -> Old
> > Gen if ladder style - but I'm not 100% sold on it myself :)
> >
> >
> > Thanks!
> >
> > -James
> >
> > >
> > > drm_atomic_helper_commit_hw_done(&state->base);
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index 6b0a646f0170..82f8df65347e 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -512,6 +512,9 @@ struct intel_atomic_state {
> > > struct i915_sw_fence commit_ready;
> > >
> > > struct llist_node freed;
> > > +
> > > + /* Gen11+ only */
> > > + u32 qgv_points_mask;
> > > };
> > >
> > > struct intel_plane_state {
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index fcf7423075ef..383de77a7b73 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1652,6 +1652,8 @@ struct drm_i915_private {
> > > u8 num_planes;
> > > } max_bw[6];
> > >
> > > + u32 qgv_points_mask;
> > > +
> > > struct drm_private_obj bw_obj;
> > >
> > > struct intel_runtime_pm runtime_pm;
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index e752de9470bd..c78ee180c1aa 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -8854,6 +8854,7 @@ enum {
> > > #define ICL_PCODE_MEM_SUBSYSYSTEM_INFO 0xd
> > > #define ICL_PCODE_MEM_SS_READ_GLOBAL_INFO (0x0 << 8)
> > > #define ICL_PCODE_MEM_SS_READ_QGV_POINT_INFO(point) (((poin
> > > t) << 16) | (0x1 << 8))
> > > +#define ICL_PCODE_SAGV_DE_MEM_SS_CONFIG 0xe
> > > #define GEN6_PCODE_READ_D_COMP 0x10
> > > #define GEN6_PCODE_WRITE_D_COMP 0x11
> > > #define HSW_PCODE_DE_WRITE_FREQ_REQ 0x17
> > > @@ -8865,6 +8866,8 @@ enum {
> > > #define GEN9_SAGV_DISABLE 0x0
> > > #define GEN9_SAGV_IS_DISABLED 0x1
> > > #define GEN9_SAGV_ENABLE 0x3
> > > +#define GEN11_PCODE_POINTS_RESTRICTED 0x0
> > > +#define GEN11_PCODE_POINTS_RESTRICTED_MASK 0x1
> > > #define GEN6_PCODE_DATA _MMIO(0x138128)
> > > #define GEN6_PCODE_FREQ_IA_RATIO_SHIFT 8
> > > #define GEN6_PCODE_FREQ_RING_RATIO_SHIFT 16
> > > --
> > > 2.17.1
> > >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
