| Message ID | 20230320165945.3564891-4-jouni.hogander@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | High refresh rate PSR fixes | expand |
On Mon, Mar 20, 2023 at 06:59:44PM +0200, Jouni Högander wrote: > Ensure vblank >= psr2 vblank > where > Psr2 vblank = PSR2_CTL Block Count Number maximum line count. > > Bspec: 71580, 49274 > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > --- > drivers/gpu/drm/i915/display/intel_psr.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > index 1050d777a108..1b40d9c73c18 100644 > --- a/drivers/gpu/drm/i915/display/intel_psr.c > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > @@ -958,6 +958,14 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp, > return false; > } > > + /* Vblank >= PSR2_CTL Block Count Number maximum line count */ > + if (crtc_state->hw.adjusted_mode.crtc_vblank_end - > + crtc_state->hw.adjusted_mode.crtc_vblank_start < 12) { Why 12? Shouldn't it be based on the wake_lines/BLOCK_COUNT_NUM stuff? If so I would suggest we try someting like this: psr2_block_count_lines() { return ...wake_lines... ? 12 : 8; } psr2_block_count() { return psr2_block_count_lines() / 4; } if (vblank_lengh < psr2_block_count_lines()) fail; if (psr_block_count() > 2) val |= BLOCK_COUNT_NUM_3; else val |= BLOCK_COUNT_NUM_2; > + drm_dbg_kms(&dev_priv->drm, > + "PSR2 not enabled, too short vblank time\n"); > + return false; > + } > + > if (HAS_PSR2_SEL_FETCH(dev_priv)) { > if (!intel_psr2_sel_fetch_config_valid(intel_dp, crtc_state) && > !HAS_PSR_HW_TRACKING(dev_priv)) { > -- > 2.34.1
On Tue, 2023-03-21 at 17:43 +0200, Ville Syrjälä wrote: > On Mon, Mar 20, 2023 at 06:59:44PM +0200, Jouni Högander wrote: > > Ensure vblank >= psr2 vblank > > where > > Psr2 vblank = PSR2_CTL Block Count Number maximum line count. > > > > Bspec: 71580, 49274 > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_psr.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > b/drivers/gpu/drm/i915/display/intel_psr.c > > index 1050d777a108..1b40d9c73c18 100644 > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > @@ -958,6 +958,14 @@ static bool intel_psr2_config_valid(struct > > intel_dp *intel_dp, > > return false; > > } > > > > + /* Vblank >= PSR2_CTL Block Count Number maximum line count > > */ > > + if (crtc_state->hw.adjusted_mode.crtc_vblank_end - > > + crtc_state->hw.adjusted_mode.crtc_vblank_start < 12) { > > Why 12? Shouldn't it be based on the wake_lines/BLOCK_COUNT_NUM > stuff? I took this directly from Bspec. I think your suggestions make sense. I will experiment them and come back on this. > > > If so I would suggest we try someting like this: > > psr2_block_count_lines() > { > return ...wake_lines... ? 12 : 8; > } > > psr2_block_count() > { > return psr2_block_count_lines() / 4; > } > > if (vblank_lengh < psr2_block_count_lines()) > fail; > > if (psr_block_count() > 2) > val |= BLOCK_COUNT_NUM_3; > else > val |= BLOCK_COUNT_NUM_2; > > > + drm_dbg_kms(&dev_priv->drm, > > + "PSR2 not enabled, too short vblank > > time\n"); > > + return false; > > + } > > + > > if (HAS_PSR2_SEL_FETCH(dev_priv)) { > > if (!intel_psr2_sel_fetch_config_valid(intel_dp, > > crtc_state) && > > !HAS_PSR_HW_TRACKING(dev_priv)) { > > -- > > 2.34.1 >
On Tue, Mar 21, 2023 at 04:14:57PM +0000, Hogander, Jouni wrote: > On Tue, 2023-03-21 at 17:43 +0200, Ville Syrjälä wrote: > > On Mon, Mar 20, 2023 at 06:59:44PM +0200, Jouni Högander wrote: > > > Ensure vblank >= psr2 vblank > > > where > > > Psr2 vblank = PSR2_CTL Block Count Number maximum line count. > > > > > > Bspec: 71580, 49274 > > > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_psr.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > > b/drivers/gpu/drm/i915/display/intel_psr.c > > > index 1050d777a108..1b40d9c73c18 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > @@ -958,6 +958,14 @@ static bool intel_psr2_config_valid(struct > > > intel_dp *intel_dp, > > > return false; > > > } > > > > > > + /* Vblank >= PSR2_CTL Block Count Number maximum line count > > > */ > > > + if (crtc_state->hw.adjusted_mode.crtc_vblank_end - > > > + crtc_state->hw.adjusted_mode.crtc_vblank_start < 12) { > > > > Why 12? Shouldn't it be based on the wake_lines/BLOCK_COUNT_NUM > > stuff? > > I took this directly from Bspec. I think your suggestions make sense. I > will experiment them and come back on this. > > > > > > > If so I would suggest we try someting like this: > > > > psr2_block_count_lines() > > { > > return ...wake_lines... ? 12 : 8; I guess we could even make that 'roundup(max(wake_lines), 4)' to be more future proof. Hmm, except that might not be all that future proof if the hardware didn't support all block size between the min/max. Eg. if it only supported 2,3,5 blocks. So I guess we might want this thing to return only actually supported numbers. > > } > > > > psr2_block_count() > > { > > return psr2_block_count_lines() / 4; > > } > > > > if (vblank_lengh < psr2_block_count_lines()) > > fail; > > > > if (psr_block_count() > 2) > > val |= BLOCK_COUNT_NUM_3; > > else > > val |= BLOCK_COUNT_NUM_2; > > > > > + drm_dbg_kms(&dev_priv->drm, > > > + "PSR2 not enabled, too short vblank > > > time\n"); > > > + return false; > > > + } > > > + > > > if (HAS_PSR2_SEL_FETCH(dev_priv)) { > > > if (!intel_psr2_sel_fetch_config_valid(intel_dp, > > > crtc_state) && > > > !HAS_PSR_HW_TRACKING(dev_priv)) { > > > -- > > > 2.34.1 > > >
On Tue, Mar 21, 2023 at 04:14:57PM +0000, Hogander, Jouni wrote: > On Tue, 2023-03-21 at 17:43 +0200, Ville Syrjälä wrote: > > On Mon, Mar 20, 2023 at 06:59:44PM +0200, Jouni Högander wrote: > > > Ensure vblank >= psr2 vblank > > > where > > > Psr2 vblank = PSR2_CTL Block Count Number maximum line count. > > > > > > Bspec: 71580, 49274 > > > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_psr.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > > b/drivers/gpu/drm/i915/display/intel_psr.c > > > index 1050d777a108..1b40d9c73c18 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > @@ -958,6 +958,14 @@ static bool intel_psr2_config_valid(struct > > > intel_dp *intel_dp, > > > return false; > > > } > > > > > > + /* Vblank >= PSR2_CTL Block Count Number maximum line count > > > */ > > > + if (crtc_state->hw.adjusted_mode.crtc_vblank_end - > > > + crtc_state->hw.adjusted_mode.crtc_vblank_start < 12) { > > > > Why 12? Shouldn't it be based on the wake_lines/BLOCK_COUNT_NUM > > stuff? > > I took this directly from Bspec. I think your suggestions make sense. I > will experiment them and come back on this. BTW the other thing that might be a bit unclear here is whether we care about the transcoder's full vblank length, or the pipe's vblank length (as reduced by the delayed vblank stuff). If you're experimenting with this then changing the vblank delay can be done live with intel_reg (on tgl just alter TRANS_VBLANK.vblanl_start, on adl+ alter TRANS_SET_CONTEXT_LATENCY). When doing stuff like this I always just run eg. 'testdisplay -o <id>,0' to quiesce the driver as much as possible, and then 'export IGT_NO_FORCEWAKE=1' before poking the registers with intel_reg avoid the debugfs forcewake stuff from perturbing the system either. Using that approach it should be possible to determine which vblank length actually matters. Though you do need to be careful about the pkg-c latency/prefill stuff when increasing the vblank delay. So might also need to disable wm1+ (and maybe also sagv) leaving only wm0 enabled. That would allow you to push the pipe's delayed start of vblank very close to the end of vblank without getting underruns.
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c index 1050d777a108..1b40d9c73c18 100644 --- a/drivers/gpu/drm/i915/display/intel_psr.c +++ b/drivers/gpu/drm/i915/display/intel_psr.c @@ -958,6 +958,14 @@ static bool intel_psr2_config_valid(struct intel_dp *intel_dp, return false; } + /* Vblank >= PSR2_CTL Block Count Number maximum line count */ + if (crtc_state->hw.adjusted_mode.crtc_vblank_end - + crtc_state->hw.adjusted_mode.crtc_vblank_start < 12) { + drm_dbg_kms(&dev_priv->drm, + "PSR2 not enabled, too short vblank time\n"); + return false; + } + if (HAS_PSR2_SEL_FETCH(dev_priv)) { if (!intel_psr2_sel_fetch_config_valid(intel_dp, crtc_state) && !HAS_PSR_HW_TRACKING(dev_priv)) {
Ensure vblank >= psr2 vblank where Psr2 vblank = PSR2_CTL Block Count Number maximum line count. Bspec: 71580, 49274 Signed-off-by: Jouni Högander <jouni.hogander@intel.com> --- drivers/gpu/drm/i915/display/intel_psr.c | 8 ++++++++ 1 file changed, 8 insertions(+)