Re: [Intel-gfx] [PATCH v2] drm/i915: Restrict qgv points which don't have enough bandwidth.
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
> > ---
> > 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, );
> > + 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,
Re: [Intel-gfx] [PATCH v2] drm/i915: Restrict qgv points which don't have enough bandwidth.
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
> ---
> 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, );
> + 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.
> + }
> +
> + 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)
[Intel-gfx] [PATCH v2] drm/i915: Restrict qgv points which don't have enough bandwidth.
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
---
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, );
+ if (ret < 0) {
+ goto fallback;
+ }
+
+ 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;
+ } 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)) {
+ 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;
+ }
+
+ 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)(((point) <<
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@lists.freedesktop.org
