* [Intel-gfx] [PATCH] drm/i915/display: Support PSR entry VSC packet to be transmitted one frame earlier
@ 2023-10-25 9:46 Mika Kahola
2023-10-25 12:08 ` Ville Syrjälä
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Mika Kahola @ 2023-10-25 9:46 UTC (permalink / raw)
To: intel-gfx
Display driver shall read DPCD 00071h[3:1] during configuration
to get PSR setup time. This register provides the setup time
requirement on the VSC SDP entry packet. If setup time cannot be
met with the current timings
(e.g., PSR setup time + other blanking requirements > blanking time),
driver should enable sending VSC SDP one frame earlier before sending
the capture frame.
BSpec: 69895 (PSR Entry Setup Frames 17:16)
Signed-off-by: Mika Kahola <mika.kahola@intel.com>
---
.../drm/i915/display/intel_display_types.h | 1 +
drivers/gpu/drm/i915/display/intel_psr.c | 35 ++++++++++++++++---
drivers/gpu/drm/i915/display/intel_psr_regs.h | 2 ++
3 files changed, 33 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 65ea37fe8cff..a0bcab6f2bec 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1710,6 +1710,7 @@ struct intel_psr {
u32 dc3co_exitline;
u32 dc3co_exit_delay;
struct delayed_work dc3co_work;
+ u8 entry_setup_frames;
};
struct intel_dp {
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 4f1f31fc9529..0acb4edae128 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -592,6 +592,9 @@ static void intel_psr_enable_sink(struct intel_dp *intel_dp)
if (intel_dp->psr.req_psr2_sdp_prior_scanline)
dpcd_val |= DP_PSR_SU_REGION_SCANLINE_CAPTURE;
+ if (intel_dp->psr.entry_setup_frames > 0)
+ dpcd_val |= DP_PSR_FRAME_CAPTURE;
+
drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, dpcd_val);
drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
@@ -690,6 +693,9 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
if (DISPLAY_VER(dev_priv) >= 8)
val |= EDP_PSR_CRC_ENABLE;
+ if (intel_dp->psr.entry_setup_frames > 0)
+ val |= EDP_PSR_ENTRY_SETUP_FRAMES(intel_dp->psr.entry_setup_frames);
+
intel_de_rmw(dev_priv, psr_ctl_reg(dev_priv, cpu_transcoder),
~EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK, val);
}
@@ -731,6 +737,7 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
{
struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
+ u8 frames_before_su_entry;
u32 val = EDP_PSR2_ENABLE;
val |= EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
@@ -741,7 +748,10 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
if (DISPLAY_VER(dev_priv) >= 10 && DISPLAY_VER(dev_priv) <= 12)
val |= EDP_Y_COORDINATE_ENABLE;
- val |= EDP_PSR2_FRAME_BEFORE_SU(max_t(u8, intel_dp->psr.sink_sync_latency + 1, 2));
+ frames_before_su_entry = max_t(u8,
+ intel_dp->psr.sink_sync_latency + 1,
+ 2);
+ val |= EDP_PSR2_FRAME_BEFORE_SU(frames_before_su_entry);
val |= intel_psr2_get_tp_time(intel_dp);
if (DISPLAY_VER(dev_priv) >= 12) {
@@ -785,6 +795,14 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
if (intel_dp->psr.req_psr2_sdp_prior_scanline)
val |= EDP_PSR2_SU_SDP_SCANLINE;
+ /* Entry setup frames must be at least 1 less than frames before SU entry */
+ if (intel_dp->psr.entry_setup_frames > 0) {
+ val |= EDP_PSR_ENTRY_SETUP_FRAMES(intel_dp->psr.entry_setup_frames);
+
+ if (intel_dp->psr.entry_setup_frames >= frames_before_su_entry)
+ val |= EDP_PSR2_FRAME_BEFORE_SU(frames_before_su_entry + 1);
+ }
+
if (intel_dp->psr.psr2_sel_fetch_enabled) {
u32 tmp;
@@ -1252,10 +1270,17 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
if (intel_usecs_to_scanlines(adjusted_mode, psr_setup_time) >
adjusted_mode->crtc_vtotal - adjusted_mode->crtc_vdisplay - 1) {
- drm_dbg_kms(&dev_priv->drm,
- "PSR condition failed: PSR setup time (%d us) too long\n",
- psr_setup_time);
- return;
+ if (DISPLAY_VER(dev_priv) >= 20) {
+ intel_dp->psr.entry_setup_frames = 1;
+ drm_dbg_kms(&dev_priv->drm,
+ "PSR setup entry frames: %d\n",
+ intel_dp->psr.entry_setup_frames);
+ } else {
+ drm_dbg_kms(&dev_priv->drm,
+ "PSR condition failed: PSR setup time (%d us) too long\n",
+ psr_setup_time);
+ return;
+ }
}
crtc_state->has_psr = true;
diff --git a/drivers/gpu/drm/i915/display/intel_psr_regs.h b/drivers/gpu/drm/i915/display/intel_psr_regs.h
index d39951383c92..9414c4de5f6e 100644
--- a/drivers/gpu/drm/i915/display/intel_psr_regs.h
+++ b/drivers/gpu/drm/i915/display/intel_psr_regs.h
@@ -35,6 +35,8 @@
#define EDP_PSR_MIN_LINK_ENTRY_TIME_0_LINES REG_FIELD_PREP(EDP_PSR_MIN_LINK_ENTRY_TIME_MASK, 3)
#define EDP_PSR_MAX_SLEEP_TIME_MASK REG_GENMASK(24, 20)
#define EDP_PSR_MAX_SLEEP_TIME(x) REG_FIELD_PREP(EDP_PSR_MAX_SLEEP_TIME_MASK, (x))
+#define EDP_PSR_ENTRY_SETUP_FRAMES_MASK REG_GENMASK(17, 16)
+#define EDP_PSR_ENTRY_SETUP_FRAMES(x) REG_FIELD_PREP(EDP_PSR_ENTRY_SETUP_FRAMES_MASK, (x))
#define EDP_PSR_SKIP_AUX_EXIT REG_BIT(12)
#define EDP_PSR_TP_MASK REG_BIT(11)
#define EDP_PSR_TP_TP1_TP2 REG_FIELD_PREP(EDP_PSR_TP_MASK, 0)
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/display: Support PSR entry VSC packet to be transmitted one frame earlier
2023-10-25 9:46 [Intel-gfx] [PATCH] drm/i915/display: Support PSR entry VSC packet to be transmitted one frame earlier Mika Kahola
@ 2023-10-25 12:08 ` Ville Syrjälä
2023-10-25 13:05 ` Hogander, Jouni
2023-10-25 18:40 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for " Patchwork
2 siblings, 0 replies; 5+ messages in thread
From: Ville Syrjälä @ 2023-10-25 12:08 UTC (permalink / raw)
To: Mika Kahola; +Cc: intel-gfx
On Wed, Oct 25, 2023 at 12:46:00PM +0300, Mika Kahola wrote:
> Display driver shall read DPCD 00071h[3:1] during configuration
> to get PSR setup time. This register provides the setup time
> requirement on the VSC SDP entry packet. If setup time cannot be
> met with the current timings
> (e.g., PSR setup time + other blanking requirements > blanking time),
> driver should enable sending VSC SDP one frame earlier before sending
> the capture frame.
>
> BSpec: 69895 (PSR Entry Setup Frames 17:16)
>
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> ---
> .../drm/i915/display/intel_display_types.h | 1 +
> drivers/gpu/drm/i915/display/intel_psr.c | 35 ++++++++++++++++---
> drivers/gpu/drm/i915/display/intel_psr_regs.h | 2 ++
> 3 files changed, 33 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 65ea37fe8cff..a0bcab6f2bec 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1710,6 +1710,7 @@ struct intel_psr {
> u32 dc3co_exitline;
> u32 dc3co_exit_delay;
> struct delayed_work dc3co_work;
> + u8 entry_setup_frames;
> };
>
> struct intel_dp {
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index 4f1f31fc9529..0acb4edae128 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -592,6 +592,9 @@ static void intel_psr_enable_sink(struct intel_dp *intel_dp)
> if (intel_dp->psr.req_psr2_sdp_prior_scanline)
> dpcd_val |= DP_PSR_SU_REGION_SCANLINE_CAPTURE;
>
> + if (intel_dp->psr.entry_setup_frames > 0)
> + dpcd_val |= DP_PSR_FRAME_CAPTURE;
> +
> drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, dpcd_val);
>
> drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
> @@ -690,6 +693,9 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
> if (DISPLAY_VER(dev_priv) >= 8)
> val |= EDP_PSR_CRC_ENABLE;
>
> + if (intel_dp->psr.entry_setup_frames > 0)
> + val |= EDP_PSR_ENTRY_SETUP_FRAMES(intel_dp->psr.entry_setup_frames);
> +
> intel_de_rmw(dev_priv, psr_ctl_reg(dev_priv, cpu_transcoder),
> ~EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK, val);
> }
> @@ -731,6 +737,7 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
> {
> struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
> + u8 frames_before_su_entry;
> u32 val = EDP_PSR2_ENABLE;
>
> val |= EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> @@ -741,7 +748,10 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
> if (DISPLAY_VER(dev_priv) >= 10 && DISPLAY_VER(dev_priv) <= 12)
> val |= EDP_Y_COORDINATE_ENABLE;
>
> - val |= EDP_PSR2_FRAME_BEFORE_SU(max_t(u8, intel_dp->psr.sink_sync_latency + 1, 2));
> + frames_before_su_entry = max_t(u8,
> + intel_dp->psr.sink_sync_latency + 1,
> + 2);
I would put that (and the setup_frames +1) into its own function.
> + val |= EDP_PSR2_FRAME_BEFORE_SU(frames_before_su_entry);
> val |= intel_psr2_get_tp_time(intel_dp);
>
> if (DISPLAY_VER(dev_priv) >= 12) {
> @@ -785,6 +795,14 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
> if (intel_dp->psr.req_psr2_sdp_prior_scanline)
> val |= EDP_PSR2_SU_SDP_SCANLINE;
>
> + /* Entry setup frames must be at least 1 less than frames before SU entry */
> + if (intel_dp->psr.entry_setup_frames > 0) {
> + val |= EDP_PSR_ENTRY_SETUP_FRAMES(intel_dp->psr.entry_setup_frames);
You're writitng that into the wrong registers.
> +
> + if (intel_dp->psr.entry_setup_frames >= frames_before_su_entry)
> + val |= EDP_PSR2_FRAME_BEFORE_SU(frames_before_su_entry + 1);
> + }
> +
> if (intel_dp->psr.psr2_sel_fetch_enabled) {
> u32 tmp;
>
> @@ -1252,10 +1270,17 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
>
> if (intel_usecs_to_scanlines(adjusted_mode, psr_setup_time) >
> adjusted_mode->crtc_vtotal - adjusted_mode->crtc_vdisplay - 1) {
> - drm_dbg_kms(&dev_priv->drm,
> - "PSR condition failed: PSR setup time (%d us) too long\n",
> - psr_setup_time);
> - return;
> + if (DISPLAY_VER(dev_priv) >= 20) {
> + intel_dp->psr.entry_setup_frames = 1;
I don't see where you're clearing this to 0.
I would extract the whole setup_time -> setup_frames conversion into
its own function and just do a setup_frames vs. platform check here.
> + drm_dbg_kms(&dev_priv->drm,
> + "PSR setup entry frames: %d\n",
> + intel_dp->psr.entry_setup_frames);
> + } else {
> + drm_dbg_kms(&dev_priv->drm,
> + "PSR condition failed: PSR setup time (%d us) too long\n",
> + psr_setup_time);
> + return;
> + }
> }
>
> crtc_state->has_psr = true;
> diff --git a/drivers/gpu/drm/i915/display/intel_psr_regs.h b/drivers/gpu/drm/i915/display/intel_psr_regs.h
> index d39951383c92..9414c4de5f6e 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr_regs.h
> +++ b/drivers/gpu/drm/i915/display/intel_psr_regs.h
> @@ -35,6 +35,8 @@
> #define EDP_PSR_MIN_LINK_ENTRY_TIME_0_LINES REG_FIELD_PREP(EDP_PSR_MIN_LINK_ENTRY_TIME_MASK, 3)
> #define EDP_PSR_MAX_SLEEP_TIME_MASK REG_GENMASK(24, 20)
> #define EDP_PSR_MAX_SLEEP_TIME(x) REG_FIELD_PREP(EDP_PSR_MAX_SLEEP_TIME_MASK, (x))
> +#define EDP_PSR_ENTRY_SETUP_FRAMES_MASK REG_GENMASK(17, 16)
> +#define EDP_PSR_ENTRY_SETUP_FRAMES(x) REG_FIELD_PREP(EDP_PSR_ENTRY_SETUP_FRAMES_MASK, (x))
> #define EDP_PSR_SKIP_AUX_EXIT REG_BIT(12)
> #define EDP_PSR_TP_MASK REG_BIT(11)
> #define EDP_PSR_TP_TP1_TP2 REG_FIELD_PREP(EDP_PSR_TP_MASK, 0)
> --
> 2.34.1
--
Ville Syrjälä
Intel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/display: Support PSR entry VSC packet to be transmitted one frame earlier
2023-10-25 9:46 [Intel-gfx] [PATCH] drm/i915/display: Support PSR entry VSC packet to be transmitted one frame earlier Mika Kahola
2023-10-25 12:08 ` Ville Syrjälä
@ 2023-10-25 13:05 ` Hogander, Jouni
2023-10-26 10:42 ` Kahola, Mika
2023-10-25 18:40 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for " Patchwork
2 siblings, 1 reply; 5+ messages in thread
From: Hogander, Jouni @ 2023-10-25 13:05 UTC (permalink / raw)
To: Kahola, Mika, intel-gfx
On Wed, 2023-10-25 at 12:46 +0300, Mika Kahola wrote:
> Display driver shall read DPCD 00071h[3:1] during configuration
> to get PSR setup time. This register provides the setup time
> requirement on the VSC SDP entry packet. If setup time cannot be
> met with the current timings
> (e.g., PSR setup time + other blanking requirements > blanking time),
> driver should enable sending VSC SDP one frame earlier before sending
> the capture frame.
>
> BSpec: 69895 (PSR Entry Setup Frames 17:16)
>
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> ---
> .../drm/i915/display/intel_display_types.h | 1 +
> drivers/gpu/drm/i915/display/intel_psr.c | 35 ++++++++++++++++-
> --
> drivers/gpu/drm/i915/display/intel_psr_regs.h | 2 ++
> 3 files changed, 33 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 65ea37fe8cff..a0bcab6f2bec 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1710,6 +1710,7 @@ struct intel_psr {
> u32 dc3co_exitline;
> u32 dc3co_exit_delay;
> struct delayed_work dc3co_work;
> + u8 entry_setup_frames;
eDP spec is speaking about 1 frame. Our HW seem to have possibility to
have several.
> };
>
> struct intel_dp {
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index 4f1f31fc9529..0acb4edae128 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -592,6 +592,9 @@ static void intel_psr_enable_sink(struct intel_dp
> *intel_dp)
> if (intel_dp->psr.req_psr2_sdp_prior_scanline)
> dpcd_val |= DP_PSR_SU_REGION_SCANLINE_CAPTURE;
>
> + if (intel_dp->psr.entry_setup_frames > 0)
> + dpcd_val |= DP_PSR_FRAME_CAPTURE;
> +
> drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, dpcd_val);
>
> drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
> DP_SET_POWER_D0);
> @@ -690,6 +693,9 @@ static void hsw_activate_psr1(struct intel_dp
> *intel_dp)
> if (DISPLAY_VER(dev_priv) >= 8)
> val |= EDP_PSR_CRC_ENABLE;
>
> + if (intel_dp->psr.entry_setup_frames > 0)
> + val |= EDP_PSR_ENTRY_SETUP_FRAMES(intel_dp-
> >psr.entry_setup_frames);
> +
> intel_de_rmw(dev_priv, psr_ctl_reg(dev_priv, cpu_transcoder),
> ~EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK, val);
> }
> @@ -731,6 +737,7 @@ static void hsw_activate_psr2(struct intel_dp
> *intel_dp)
> {
> struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
> + u8 frames_before_su_entry;
> u32 val = EDP_PSR2_ENABLE;
>
> val |=
> EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> @@ -741,7 +748,10 @@ static void hsw_activate_psr2(struct intel_dp
> *intel_dp)
> if (DISPLAY_VER(dev_priv) >= 10 && DISPLAY_VER(dev_priv) <=
> 12)
> val |= EDP_Y_COORDINATE_ENABLE;
>
> - val |= EDP_PSR2_FRAME_BEFORE_SU(max_t(u8, intel_dp-
> >psr.sink_sync_latency + 1, 2));
> + frames_before_su_entry = max_t(u8,
> + intel_dp-
> >psr.sink_sync_latency + 1,
> + 2);
> + val |= EDP_PSR2_FRAME_BEFORE_SU(frames_before_su_entry);
I think you want to do this only once. Below or here.
> val |= intel_psr2_get_tp_time(intel_dp);
>
> if (DISPLAY_VER(dev_priv) >= 12) {
> @@ -785,6 +795,14 @@ static void hsw_activate_psr2(struct intel_dp
> *intel_dp)
> if (intel_dp->psr.req_psr2_sdp_prior_scanline)
> val |= EDP_PSR2_SU_SDP_SCANLINE;
>
> + /* Entry setup frames must be at least 1 less than frames
> before SU entry */
> + if (intel_dp->psr.entry_setup_frames > 0) {
> + val |= EDP_PSR_ENTRY_SETUP_FRAMES(intel_dp-
> >psr.entry_setup_frames);
This is not the correct register. You want to have this in PSR_CTL (not
PSR2_CTL).
> +
> + if (intel_dp->psr.entry_setup_frames >=
> frames_before_su_entry)
> + val |=
> EDP_PSR2_FRAME_BEFORE_SU(frames_before_su_entry + 1);
> + }
> +
Here you are ORing second time. Please do it only once.
> if (intel_dp->psr.psr2_sel_fetch_enabled) {
> u32 tmp;
>
> @@ -1252,10 +1270,17 @@ void intel_psr_compute_config(struct intel_dp
> *intel_dp,
>
> if (intel_usecs_to_scanlines(adjusted_mode, psr_setup_time) >
> adjusted_mode->crtc_vtotal - adjusted_mode->crtc_vdisplay
> - 1) {
> - drm_dbg_kms(&dev_priv->drm,
> - "PSR condition failed: PSR setup time (%d
> us) too long\n",
> - psr_setup_time);
> - return;
> + if (DISPLAY_VER(dev_priv) >= 20) {
> + intel_dp->psr.entry_setup_frames = 1;
> + drm_dbg_kms(&dev_priv->drm,
> + "PSR setup entry frames: %d\n",
> + intel_dp-
> >psr.entry_setup_frames);
> + } else {
> + drm_dbg_kms(&dev_priv->drm,
> + "PSR condition failed: PSR setup
> time (%d us) too long\n",
> + psr_setup_time);
> + return;
> + }
Maybe you could add separate helper for this. E.g.
_compute_psr_setup_frames().
> }
>
> crtc_state->has_psr = true;
> diff --git a/drivers/gpu/drm/i915/display/intel_psr_regs.h
> b/drivers/gpu/drm/i915/display/intel_psr_regs.h
> index d39951383c92..9414c4de5f6e 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr_regs.h
> +++ b/drivers/gpu/drm/i915/display/intel_psr_regs.h
> @@ -35,6 +35,8 @@
> #define
> EDP_PSR_MIN_LINK_ENTRY_TIME_0_LINES REG_FIELD_PREP(EDP_PSR_MIN_LINK_
> ENTRY_TIME_MASK, 3)
> #define EDP_PSR_MAX_SLEEP_TIME_MASK REG_GENMASK(24, 20)
> #define
> EDP_PSR_MAX_SLEEP_TIME(x) REG_FIELD_PREP(EDP_PSR_MAX_SLEEP
> _TIME_MASK, (x))
> +#define EDP_PSR_ENTRY_SETUP_FRAMES_MASK REG_GENMASK(17, 16)
> +#define
> EDP_PSR_ENTRY_SETUP_FRAMES(x) REG_FIELD_PREP(EDP_PSR_E
Maybe LNL prefix here could clarify that this is only in LNL and
beyond.
BR,
Jouni Högander
> NTRY_SETUP_FRAMES_MASK, (x))
> #define EDP_PSR_SKIP_AUX_EXIT REG_BIT(12)
> #define EDP_PSR_TP_MASK REG_BIT(11)
> #define
> EDP_PSR_TP_TP1_TP2 REG_FIELD_PREP(EDP_PSR_TP_MASK,
> 0)
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/display: Support PSR entry VSC packet to be transmitted one frame earlier
2023-10-25 9:46 [Intel-gfx] [PATCH] drm/i915/display: Support PSR entry VSC packet to be transmitted one frame earlier Mika Kahola
2023-10-25 12:08 ` Ville Syrjälä
2023-10-25 13:05 ` Hogander, Jouni
@ 2023-10-25 18:40 ` Patchwork
2 siblings, 0 replies; 5+ messages in thread
From: Patchwork @ 2023-10-25 18:40 UTC (permalink / raw)
To: Mika Kahola; +Cc: intel-gfx
[-- Attachment #1: Type: text/plain, Size: 5010 bytes --]
== Series Details ==
Series: drm/i915/display: Support PSR entry VSC packet to be transmitted one frame earlier
URL : https://patchwork.freedesktop.org/series/125558/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_13786 -> Patchwork_125558v1
====================================================
Summary
-------
**FAILURE**
Serious unknown changes coming with Patchwork_125558v1 absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_125558v1, please notify your bug team (lgci.bug.filing@intel.com) to allow them
to document this new failure mode, which will reduce false positives in CI.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125558v1/index.html
Participating hosts (38 -> 37)
------------------------------
Additional (1): bat-adlp-11
Missing (2): fi-snb-2520m fi-bsw-n3050
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in Patchwork_125558v1:
### IGT changes ###
#### Possible regressions ####
* igt@i915_selftest@live@hangcheck:
- fi-ivb-3770: [PASS][1] -> [INCOMPLETE][2]
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13786/fi-ivb-3770/igt@i915_selftest@live@hangcheck.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125558v1/fi-ivb-3770/igt@i915_selftest@live@hangcheck.html
Known issues
------------
Here are the changes found in Patchwork_125558v1 that come from known issues:
### CI changes ###
#### Issues hit ####
* boot:
- bat-adlp-11: NOTRUN -> [FAIL][3] ([i915#8293])
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125558v1/bat-adlp-11/boot.html
#### Possible fixes ####
* boot:
- bat-jsl-1: [FAIL][4] ([i915#8293]) -> [PASS][5]
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13786/bat-jsl-1/boot.html
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125558v1/bat-jsl-1/boot.html
### IGT changes ###
#### Issues hit ####
* igt@debugfs_test@basic-hwmon:
- bat-jsl-1: NOTRUN -> [SKIP][6] ([i915#9318])
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125558v1/bat-jsl-1/igt@debugfs_test@basic-hwmon.html
* igt@gem_huc_copy@huc-copy:
- bat-jsl-1: NOTRUN -> [SKIP][7] ([i915#2190])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125558v1/bat-jsl-1/igt@gem_huc_copy@huc-copy.html
* igt@gem_lmem_swapping@verify-random:
- bat-jsl-1: NOTRUN -> [SKIP][8] ([i915#4613]) +3 other tests skip
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125558v1/bat-jsl-1/igt@gem_lmem_swapping@verify-random.html
* igt@i915_selftest@live@gt_heartbeat:
- fi-apl-guc: [PASS][9] -> [DMESG-FAIL][10] ([i915#5334])
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13786/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125558v1/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html
* igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
- bat-jsl-1: NOTRUN -> [SKIP][11] ([i915#4103]) +1 other test skip
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125558v1/bat-jsl-1/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
* igt@kms_dsc@dsc-basic:
- bat-jsl-1: NOTRUN -> [SKIP][12] ([i915#3555]) +1 other test skip
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125558v1/bat-jsl-1/igt@kms_dsc@dsc-basic.html
* igt@kms_force_connector_basic@force-load-detect:
- bat-jsl-1: NOTRUN -> [SKIP][13] ([fdo#109285])
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125558v1/bat-jsl-1/igt@kms_force_connector_basic@force-load-detect.html
[fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
[i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
[i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
[i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
[i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
[i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
[i915#8293]: https://gitlab.freedesktop.org/drm/intel/issues/8293
[i915#9318]: https://gitlab.freedesktop.org/drm/intel/issues/9318
Build changes
-------------
* Linux: CI_DRM_13786 -> Patchwork_125558v1
CI-20190529: 20190529
CI_DRM_13786: e8d777a5e7e0ec452142ad0073022733f99c1eb7 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_7557: 18fc864d68d382847596594d7eb3488f2c8fb45e @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_125558v1: e8d777a5e7e0ec452142ad0073022733f99c1eb7 @ git://anongit.freedesktop.org/gfx-ci/linux
### Linux commits
d940fd43c9f3 drm/i915/display: Support PSR entry VSC packet to be transmitted one frame earlier
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_125558v1/index.html
[-- Attachment #2: Type: text/html, Size: 5938 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/display: Support PSR entry VSC packet to be transmitted one frame earlier
2023-10-25 13:05 ` Hogander, Jouni
@ 2023-10-26 10:42 ` Kahola, Mika
0 siblings, 0 replies; 5+ messages in thread
From: Kahola, Mika @ 2023-10-26 10:42 UTC (permalink / raw)
To: Hogander, Jouni, intel-gfx
> -----Original Message-----
> From: Hogander, Jouni <jouni.hogander@intel.com>
> Sent: Wednesday, October 25, 2023 4:05 PM
> To: Kahola, Mika <mika.kahola@intel.com>; intel-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/i915/display: Support PSR entry VSC packet to be transmitted one frame earlier
>
> On Wed, 2023-10-25 at 12:46 +0300, Mika Kahola wrote:
> > Display driver shall read DPCD 00071h[3:1] during configuration to get
> > PSR setup time. This register provides the setup time requirement on
> > the VSC SDP entry packet. If setup time cannot be met with the current
> > timings (e.g., PSR setup time + other blanking requirements > blanking
> > time), driver should enable sending VSC SDP one frame earlier before
> > sending the capture frame.
> >
> > BSpec: 69895 (PSR Entry Setup Frames 17:16)
> >
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > ---
> > .../drm/i915/display/intel_display_types.h | 1 +
> > drivers/gpu/drm/i915/display/intel_psr.c | 35 ++++++++++++++++-
> > --
> > drivers/gpu/drm/i915/display/intel_psr_regs.h | 2 ++
> > 3 files changed, 33 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 65ea37fe8cff..a0bcab6f2bec 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1710,6 +1710,7 @@ struct intel_psr {
> > u32 dc3co_exitline;
> > u32 dc3co_exit_delay;
> > struct delayed_work dc3co_work;
> > + u8 entry_setup_frames;
>
> eDP spec is speaking about 1 frame. Our HW seem to have possibility to have several.
That's true. The bitfield is two bits wide so anything from zero to three frames are, in theory, allowed.
Since the description talks only having one frame earlier, I sticked to this idea.
>
> > };
> >
> > struct intel_dp {
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 4f1f31fc9529..0acb4edae128 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -592,6 +592,9 @@ static void intel_psr_enable_sink(struct intel_dp
> > *intel_dp)
> > if (intel_dp->psr.req_psr2_sdp_prior_scanline)
> > dpcd_val |= DP_PSR_SU_REGION_SCANLINE_CAPTURE;
> >
> > + if (intel_dp->psr.entry_setup_frames > 0)
> > + dpcd_val |= DP_PSR_FRAME_CAPTURE;
> > +
> > drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, dpcd_val);
> >
> > drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
> > DP_SET_POWER_D0); @@ -690,6 +693,9 @@ static void
> > hsw_activate_psr1(struct intel_dp
> > *intel_dp)
> > if (DISPLAY_VER(dev_priv) >= 8)
> > val |= EDP_PSR_CRC_ENABLE;
> >
> > + if (intel_dp->psr.entry_setup_frames > 0)
> > + val |= EDP_PSR_ENTRY_SETUP_FRAMES(intel_dp-
> > >psr.entry_setup_frames);
> > +
> > intel_de_rmw(dev_priv, psr_ctl_reg(dev_priv, cpu_transcoder),
> > ~EDP_PSR_RESTORE_PSR_ACTIVE_CTX_MASK, val);
> > }
> > @@ -731,6 +737,7 @@ static void hsw_activate_psr2(struct intel_dp
> > *intel_dp)
> > {
> > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
> > + u8 frames_before_su_entry;
> > u32 val = EDP_PSR2_ENABLE;
> >
> > val |=
> > EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
> > @@ -741,7 +748,10 @@ static void hsw_activate_psr2(struct intel_dp
> > *intel_dp)
> > if (DISPLAY_VER(dev_priv) >= 10 && DISPLAY_VER(dev_priv) <=
> > 12)
> > val |= EDP_Y_COORDINATE_ENABLE;
> >
> > - val |= EDP_PSR2_FRAME_BEFORE_SU(max_t(u8, intel_dp-
> > >psr.sink_sync_latency + 1, 2));
> > + frames_before_su_entry = max_t(u8,
> > + intel_dp-
> > >psr.sink_sync_latency + 1,
> > + 2);
> > + val |= EDP_PSR2_FRAME_BEFORE_SU(frames_before_su_entry);
>
> I think you want to do this only once. Below or here.
This might end up as a separate function so I will program this value only once there.
>
> > val |= intel_psr2_get_tp_time(intel_dp);
> >
> > if (DISPLAY_VER(dev_priv) >= 12) { @@ -785,6 +795,14 @@ static
> > void hsw_activate_psr2(struct intel_dp
> > *intel_dp)
> > if (intel_dp->psr.req_psr2_sdp_prior_scanline)
> > val |= EDP_PSR2_SU_SDP_SCANLINE;
> >
> > + /* Entry setup frames must be at least 1 less than frames
> > before SU entry */
> > + if (intel_dp->psr.entry_setup_frames > 0) {
> > + val |= EDP_PSR_ENTRY_SETUP_FRAMES(intel_dp-
> > >psr.entry_setup_frames);
>
> This is not the correct register. You want to have this in PSR_CTL (not PSR2_CTL).
Yes, this ends up in the wrong place. I'll fix this.
>
> > +
> > + if (intel_dp->psr.entry_setup_frames >=
> > frames_before_su_entry)
> > + val |=
> > EDP_PSR2_FRAME_BEFORE_SU(frames_before_su_entry + 1);
> > + }
> > +
>
> Here you are ORing second time. Please do it only once.
I will rewrite this so that this is done only once.
>
> > if (intel_dp->psr.psr2_sel_fetch_enabled) {
> > u32 tmp;
> >
> > @@ -1252,10 +1270,17 @@ void intel_psr_compute_config(struct intel_dp
> > *intel_dp,
> >
> > if (intel_usecs_to_scanlines(adjusted_mode, psr_setup_time) >
> > adjusted_mode->crtc_vtotal - adjusted_mode->crtc_vdisplay
> > - 1) {
> > - drm_dbg_kms(&dev_priv->drm,
> > - "PSR condition failed: PSR setup time (%d
> > us) too long\n",
> > - psr_setup_time);
> > - return;
> > + if (DISPLAY_VER(dev_priv) >= 20) {
> > + intel_dp->psr.entry_setup_frames = 1;
> > + drm_dbg_kms(&dev_priv->drm,
> > + "PSR setup entry frames: %d\n",
> > + intel_dp-
> > >psr.entry_setup_frames);
> > + } else {
> > + drm_dbg_kms(&dev_priv->drm,
> > + "PSR condition failed: PSR setup
> > time (%d us) too long\n",
> > + psr_setup_time);
> > + return;
> > + }
>
> Maybe you could add separate helper for this. E.g.
> _compute_psr_setup_frames().
Yes. That might be cleaner solution.
>
> > }
> >
> > crtc_state->has_psr = true;
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr_regs.h
> > b/drivers/gpu/drm/i915/display/intel_psr_regs.h
> > index d39951383c92..9414c4de5f6e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr_regs.h
> > +++ b/drivers/gpu/drm/i915/display/intel_psr_regs.h
> > @@ -35,6 +35,8 @@
> > #define
> > EDP_PSR_MIN_LINK_ENTRY_TIME_0_LINES REG_FIELD_PREP(EDP_PSR_MIN_LINK_
> > ENTRY_TIME_MASK, 3)
> > #define EDP_PSR_MAX_SLEEP_TIME_MASK REG_GENMASK(24, 20)
> > #define
> > EDP_PSR_MAX_SLEEP_TIME(x) REG_FIELD_PREP(EDP_PSR_MAX_SLEEP
> > _TIME_MASK, (x))
> > +#define EDP_PSR_ENTRY_SETUP_FRAMES_MASK REG_GENMASK(17, 16)
> > +#define
> > EDP_PSR_ENTRY_SETUP_FRAMES(x) REG_FIELD_PREP(EDP_PSR_E
>
> Maybe LNL prefix here could clarify that this is only in LNL and
> beyond.
Yes, this bitfield is only for LNL+. I will rename these so this will be more clearly indicated.
Thanks for review & comments!
Mika
>
> BR,
>
> Jouni Högander
>
> > NTRY_SETUP_FRAMES_MASK, (x))
> > #define EDP_PSR_SKIP_AUX_EXIT REG_BIT(12)
> > #define EDP_PSR_TP_MASK REG_BIT(11)
> > #define
> > EDP_PSR_TP_TP1_TP2 REG_FIELD_PREP(EDP_PSR_TP_MASK,
> > 0)
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-10-26 10:42 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-25 9:46 [Intel-gfx] [PATCH] drm/i915/display: Support PSR entry VSC packet to be transmitted one frame earlier Mika Kahola
2023-10-25 12:08 ` Ville Syrjälä
2023-10-25 13:05 ` Hogander, Jouni
2023-10-26 10:42 ` Kahola, Mika
2023-10-25 18:40 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for " Patchwork
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).