On Tue, Mar 12, 2019 at 10:58:41PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> Currently we disable all the watermarks above the selected max
> level for every plane. That would mean that the cursor's watermarks
> may also get modified when another plane causes the selected
> max watermark level to change. That is not so great as we would
> like to keep the cursor as indepenedent as possible to avoid
> having to throttle it in resposne to other plane activity.
>
> To avoid that let's keep the watermarks enabled even for levels
> above the max selected watermark level, iff the plane has enough
> ddb for that particular level. This way the cursor's enabled
> watermarks only depend on the cursor itself. This is safe because
> the hardware will never choose to use a watermark level unless
> all enabled planes have also enabled that level.
>
> Cc: Neel Desai <neel.desai@xxxxxxxxx>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Iirc, we also had different levels enabled for different planes with the
old algorithm that calculated DDB first and watermarks second. So
agreed; this should be very safe.
Patches 1-6 are
Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
Matt
> ---
> drivers/gpu/drm/i915/intel_pm.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index c866663b31bc..8afbc56ad89a 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4500,7 +4500,22 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
> for (level++; level <= ilk_wm_max_level(dev_priv); level++) {
> for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> wm = &cstate->wm.skl.optimal.planes[plane_id];
> - memset(&wm->wm[level], 0, sizeof(wm->wm[level]));
> +
> + /*
> + * We only disable the watermarks for each plane if
> + * they exceed the ddb allocation of said plane. This
> + * is done so that we don't end up touching cursor
> + * watermarks needlessly when some other plane reduces
> + * our max possible watermark level.
> + *
> + * Bspec has this to say about the PLANE_WM enable bit:
> + * "All the watermarks at this level for all enabled
> + * planes must be enabled before the level will be used."
> + * So this is actually safe to do.
> + */
> + if (wm->wm[level].min_ddb_alloc > total[plane_id] ||
> + wm->uv_wm[level].min_ddb_alloc > uv_total[plane_id])
> + memset(&wm->wm[level], 0, sizeof(wm->wm[level]));
>
> /*
> * Wa_1408961008:icl
> --
> 2.19.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
