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.
> >
> > + 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
