On Wed, May 13, 2020 at 12:38:15PM +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.
>
> v4:
> - Minor code refactoring, fixed few typos(thanks to James Ausmus)
> - Change the naming of qgv point
> masking/unmasking functions(James Ausmus).
> - Simplify the masking/unmasking operation itself,
> as we don't need to mask only single point per request(James Ausmus)
> - Reject and stick to highest bandwidth point if SAGV
> can't be enabled(BSpec)
>
> v5:
> - Add new mailbox reply codes, which seems to happen during boot
> time for TGL and indicate that QGV setting is not yet available.
>
> v6:
> - Increase number of supported QGV points to be in sync with BSpec.
>
> v7: - Rebased and resolved conflict to fix build failure.
> - Fix NUM_QGV_POINTS to 8 and moved that to header file(James Ausmus)
>
> v8: - Don't report an error if we can't restrict qgv points, as SAGV
> can be disabled by BIOS, which is completely legal. So don't
> make CI panic. Instead if we detect that there is only 1 QGV
> point accessible just analyze if we can fit the required bandwidth
> requirements, but no need in restricting.
>
> v9: - Fix wrong QGV transition if we have 0 planes and no SAGV
> simultaneously.
>
> v10: - Fix CDCLK corruption, because of global state getting serialized
> without modeset, which caused copying of non-calculated cdclk
> to be copied to dev_priv(thanks to Ville for the hint).
>
> v11: - Remove unneeded headers and spaces(Matthew Roper)
> - Remove unneeded intel_qgv_info qi struct from bw check and zero
> out the needed one(Matthew Roper)
> - Changed QGV error message to have more clear meaning(Matthew Roper)
> - Use state->modeset_set instead of any_ms(Matthew Roper)
> - Moved NUM_SAGV_POINTS from i915_reg.h to i915_drv.h where it's used
> - Keep using crtc_state->hw.active instead of .enable(Matthew Roper)
> - Moved unrelated changes to other patch(using latency as parameter
> for plane wm calculation, moved to SAGV refactoring patch)
>
> v12: - Fix rebase conflict with own temporary SAGV/QGV fix.
> - Remove unnecessary mask being zero check when unmasking
> qgv points as this is completely legal(Matt Roper)
> - Check if we are setting the same mask as already being set
> in hardware to prevent error from PCode.
> - Fix error message when restricting/unrestricting qgv points
> to "mask/unmask" which sounds more accurate(Matt Roper)
> - Move sagv status setting to icl_get_bw_info from atomic check
> as this should be calculated only once.(Matt Roper)
> - Edited comments for the case when we can't enable SAGV and
> use only 1 QGV point with highest bandwidth to be more
> understandable.(Matt Roper)
>
> v13: - Moved max_data_rate in bw check to closer scope(Ville Syrjälä)
> - Changed comment for zero new_mask in qgv points masking function
> to better reflect reality(Ville Syrjälä)
> - Simplified bit mask operation in qgv points masking function
> (Ville Syrjälä)
> - Moved intel_qgv_points_mask closer to gen11 SAGV disabling,
> however this still can't be under modeset condition(Ville Syrjälä)
> - Packed qgv_points_mask as u8 and moved closer to pipe_sagv_mask
> (Ville Syrjälä)
> - Extracted PCode changes to separate patch.(Ville Syrjälä)
> - Now treat num_planes 0 same as 1 to avoid confusion and
> returning max_bw as 0, which would prevent choosing QGV
> point having max bandwidth in case if SAGV is not allowed,
> as per BSpec(Ville Syrjälä)
> - Do the actual qgv_points_mask swap in the same place as
> all other global state parts like cdclk are swapped.
> In the next patch, this all will be moved to bw state as
> global state, once new global state patch series from Ville
> lands
>
> v14: - Now using global state to serialize access to qgv points
> - Added global state locking back, otherwise we seem to read
> bw state in a wrong way.
>
> v15: - Added TODO comment for near atomic global state locking in
> bw code.
>
> v16: - Fixed intel_atomic_bw_* functions to be intel_bw_* as discussed
> with Jani Nikula.
> - Take bw_state_changed flag into use.
>
> v17: - Moved qgv point related manipulations next to SAGV code, as
> those are semantically related(Ville Syrjälä)
> - Renamed those into intel_sagv_(pre)|(post)_plane_update
> (Ville Syrjälä)
>
> v18: - Move sagv related calls from commit tail into
> intel_sagv_(pre)|(post)_plane_update(Ville Syrjälä)
>
> v19: - Use intel_atomic_get_bw_(old)|(new)_state which is intended
> for commit tail stage.
>
> v20: - Return max bandwidth for 0 planes(Ville)
> - Constify old_bw_state in bw_atomic_check(Ville)
> - Removed some debugs(Ville)
> - Added data rate to debug print when no QGV points(Ville)
> - Removed some comments(Ville)
>
> v21, v22, v23: - Fixed rebase conflict
>
> v24: - Changed PCode mask to use ICL_ prefix
> v25: - Resolved rebase conflict
>
> v26: - Removed redundant NULL checks(Ville)
> - Removed redundant error prints(Ville)
>
> 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_bw.c | 137 +++++++++++++-----
> drivers/gpu/drm/i915/display/intel_bw.h | 9 ++
> .../drm/i915/display/intel_display_types.h | 3 +
> drivers/gpu/drm/i915/intel_pm.c | 54 ++++++-
> drivers/gpu/drm/i915/intel_pm.h | 2 +
> 5 files changed, 167 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> index 6e7cc3a4f1aa..d28ca4d0b1c4 100644
> --- a/drivers/gpu/drm/i915/display/intel_bw.c
> +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> @@ -8,6 +8,9 @@
> #include "intel_bw.h"
> #include "intel_display_types.h"
> #include "intel_sideband.h"
> +#include "intel_atomic.h"
> +#include "intel_pm.h"
> +
>
> /* Parameters for Qclk Geyserville (QGV) */
> struct intel_qgv_point {
> @@ -113,6 +116,26 @@ static int icl_pcode_read_qgv_point_info(struct drm_i915_private *dev_priv,
> return 0;
> }
>
> +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,
> + ICL_PCODE_POINTS_RESTRICTED_MASK,
> + ICL_PCODE_POINTS_RESTRICTED,
> + 1);
> +
> + if (ret < 0) {
> + DRM_ERROR("Failed to disable qgv points (%d)\n", ret);
Pls use the per-device logging. drm_err() or whatever it's called.
Apart from that (and the checkpatch issue ci reported) this looks
ready to go in.
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> static int icl_get_qgv_points(struct drm_i915_private *dev_priv,
> struct intel_qgv_info *qi)
> {
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
