On Fri, 2019-09-20 at 16:19 +0300, Ville Syrjälä wrote:
> On Fri, Sep 20, 2019 at 01:44:13PM +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.
> >
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_bw.c | 58
> > +++++++++++++++++++++++--
> > drivers/gpu/drm/i915/i915_reg.h | 3 ++
> > 2 files changed, 58 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c
> > b/drivers/gpu/drm/i915/display/intel_bw.c
> > index cd58e47ab7b2..7653cbdb0ee4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bw.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> > @@ -90,6 +90,26 @@ static int icl_pcode_read_qgv_point_info(struct
> > drm_i915_private *dev_priv,
> > return 0;
> > }
> >
> > +static int icl_pcode_restrict_qgv_points(struct drm_i915_private
> > *dev_priv, u32 points_mask)
> > +{
> > + int ret;
> > +
> > + /* bspec says to keep retrying for at least 1 ms */
> > + ret = skl_pcode_request(dev_priv,
> > ICL_PCODE_SAGV_DE_MEM_SS_CONFIG,
> > + points_mask,
> > + GEN11_PCODE_POINTS_RESTRICTED_MASK,
> > + GEN11_PCODE_POINTS_RESTRICTED,
> > + 1);
> > +
> > + if (ret < 0) {
> > + DRM_ERROR("Failed to disable qgv points (%d)\n", ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +
> > static int icl_get_qgv_points(struct drm_i915_private *dev_priv,
> > struct intel_qgv_info *qi)
> > {
> > @@ -354,7 +374,9 @@ int intel_bw_atomic_check(struct
> > intel_atomic_state *state)
> > unsigned int data_rate, max_data_rate;
> > unsigned int num_active_planes;
> > struct intel_crtc *crtc;
> > - int i;
> > + int i, ret;
> > + struct intel_qgv_info qi = {};
> > + u32 points_mask = 0;
> >
> > /* FIXME earlier gens need some checks too */
> > if (INTEL_GEN(dev_priv) < 11)
> > @@ -398,10 +420,40 @@ int intel_bw_atomic_check(struct
> > intel_atomic_state *state)
> > data_rate = intel_bw_data_rate(dev_priv, bw_state);
> > num_active_planes = intel_bw_num_active_planes(dev_priv,
> > bw_state);
> >
> > - max_data_rate = intel_max_data_rate(dev_priv,
> > num_active_planes);
> > -
> > data_rate = DIV_ROUND_UP(data_rate, 1000);
> >
> > + ret = icl_get_qgv_points(dev_priv, &qi);
> > + if (ret < 0) {
> > + goto fallback;
>
> If we don't have that we don't have any idea about bw limits. So
> probably just return 0 here.
>
> > + }
> > +
> > + for (i = 0; i < qi.num_points; i++) {
> > + max_data_rate = icl_max_bw(dev_priv, num_active_planes,
> > i);
> > + if (max_data_rate < data_rate) {
> > + DRM_DEBUG_KMS("QGV point %d: max bw %d required
> > %d restricted\n",
> > + i, max_data_rate, data_rate);
> > + points_mask |= 1 << i;
>
> I think just marking the accepted levels in the mask would make
> things
> simpler...
>
> > + } else
> > + DRM_DEBUG_KMS("QGV point %d: max bw %d required
> > %d unrestricted\n",
> > + i, max_data_rate, data_rate);
> > + }
> > +
> > + if (points_mask >= ((1 << qi.num_points) - 1)) {
>
> ... eg. this can then just be 'if (points_mask == 0)'
>
> > + DRM_DEBUG_KMS("Could not find any suitable QGV
> > points\n");
> > + return -EINVAL;
> > + }
> > +
> > + ret = icl_pcode_restrict_qgv_points(dev_priv, points_mask);
> > + if (ret < 0) {
> > + DRM_DEBUG_KMS("Could not restrict required gqv
> > points(%d)\n", ret);
> > + goto fallback;
>
> Seems like dead code to me.
>
> We'll need to account for the SAGV yes/no in here as well. That is,
> if
> SAGV is off due to watermarks we'll need to restrict things to the
> highest QGV point only. Also using both the QGV point restriction
> pcode command and the legacy SAGV pcode command at the same time
> sounds
> rather risky to me. I suspect pcode might not expect that. So we need
> to rework this on a slightly bigger scale.
Well, I suspected that it's not going to be that easy..
Probably you mean that this has to be somehow put in sync with
intel_disable_sagv calls, so we need to mutually exclude those
and/or take into account.
>
> > + }
> > +
> > + return 0;
> > +
> > +fallback:
> > + max_data_rate = intel_max_data_rate(dev_priv,
> > num_active_planes);
> > +
> > if (data_rate > max_data_rate) {
> > DRM_DEBUG_KMS("Bandwidth %u MB/s exceeds max available
> > %d MB/s (%d active planes)\n",
> > data_rate, max_data_rate,
> > num_active_planes);
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index bf37ecebc82f..fe327fee8781 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -8845,6 +8845,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
> > @@ -8856,6 +8857,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
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
