Re: [PATCH v3] drm/i915: Restrict qgv points which don't have enough bandwidth.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux