| Message ID | 20220401222911.199284-2-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915/display/psr: Set partial frame enable when forcing full frame fetch | expand |
Hi "José,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm-tip/drm-tip]
[also build test WARNING on v5.17 next-20220401]
[cannot apply to drm-intel/for-linux-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Jos-Roberto-de-Souza/drm-i915-display-psr-Set-partial-frame-enable-when-forcing-full-frame-fetch/20220402-062837
base: git://anongit.freedesktop.org/drm/drm-tip drm-tip
config: x86_64-randconfig-a011 (https://download.01.org/0day-ci/archive/20220402/202204020818.qEzCpUjb-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.2.0-19) 11.2.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/955b4bf1a2fd2e6652980814983464f3db8f955f
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jos-Roberto-de-Souza/drm-i915-display-psr-Set-partial-frame-enable-when-forcing-full-frame-fetch/20220402-062837
git checkout 955b4bf1a2fd2e6652980814983464f3db8f955f
# save the config file to linux build tree
mkdir build_dir
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/gpu/drm/i915/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/i915/display/intel_psr.c:2485: warning: expecting prototype for intel_psr_lock(). Prototype was for intel_psr_unlock() instead
vim +2485 drivers/gpu/drm/i915/display/intel_psr.c
2477
2478 /**
2479 * intel_psr_lock - grab psr.lock mutex
2480 * @crtc_state: the crtc state
2481 *
2482 * Release the PSR lock that was held during pipe update.
2483 */
2484 void intel_psr_unlock(const struct intel_crtc_state *crtc_state)
> 2485 {
Hi "José,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm-tip/drm-tip]
[also build test WARNING on v5.17 next-20220401]
[cannot apply to drm-intel/for-linux-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Jos-Roberto-de-Souza/drm-i915-display-psr-Set-partial-frame-enable-when-forcing-full-frame-fetch/20220402-062837
base: git://anongit.freedesktop.org/drm/drm-tip drm-tip
config: x86_64-randconfig-a005 (https://download.01.org/0day-ci/archive/20220402/202204020818.P5BApjRe-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project cc2e2b80a1f36a28fa7c96c38c2674b10868f09f)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/955b4bf1a2fd2e6652980814983464f3db8f955f
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jos-Roberto-de-Souza/drm-i915-display-psr-Set-partial-frame-enable-when-forcing-full-frame-fetch/20220402-062837
git checkout 955b4bf1a2fd2e6652980814983464f3db8f955f
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/gpu/drm/i915/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/i915/display/intel_psr.c:2485: warning: expecting prototype for intel_psr_lock(). Prototype was for intel_psr_unlock() instead
vim +2485 drivers/gpu/drm/i915/display/intel_psr.c
2477
2478 /**
2479 * intel_psr_lock - grab psr.lock mutex
2480 * @crtc_state: the crtc state
2481 *
2482 * Release the PSR lock that was held during pipe update.
2483 */
2484 void intel_psr_unlock(const struct intel_crtc_state *crtc_state)
> 2485 {
Hello, Couple of questions below. On Fri, 2022-04-01 at 15:29 -0700, José Roberto de Souza wrote: > Frontbuffer rendering and page flips can race with each other > and this can potentialy cause issues with PSR2 selective fetch. > > And because pipe/crtc updates are time sentive we can't grab the > PSR lock after intel_pipe_update_start() and before > intel_pipe_update_end(). > > So here adding the lock and unlock functions and calls, the > proper PSR2 selective fetch handling will come in a separated patch. > Have you ensured that there is no case where pipe_update_end is not called? Why did you choose to add new hooks instead of using existing intel_psr_pre_plane_update and intel_psr_post_plane_update? > Cc: Jouni Högander <jouni.hogander@intel.com> > Cc: Mika Kahola <mika.kahola@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/display/intel_crtc.c | 6 +- > drivers/gpu/drm/i915/display/intel_psr.c | 69 ++++++++++++++++++++- > -- > drivers/gpu/drm/i915/display/intel_psr.h | 5 +- > 3 files changed, 70 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c > b/drivers/gpu/drm/i915/display/intel_crtc.c > index f655c16228776..a5439182d5ae4 100644 > --- a/drivers/gpu/drm/i915/display/intel_crtc.c > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c > @@ -507,6 +507,8 @@ void intel_pipe_update_start(struct > intel_crtc_state *new_crtc_state) > VBLANK_EVASION_TI > ME_US); > max = vblank_start - 1; > > + intel_psr_lock(new_crtc_state); > + > if (min <= 0 || max <= 0) > goto irq_disable; > > @@ -518,7 +520,7 @@ void intel_pipe_update_start(struct > intel_crtc_state *new_crtc_state) > * VBL interrupts will start the PSR exit and prevent a PSR > * re-entry as well. > */ > - intel_psr_wait_for_idle(new_crtc_state); > + intel_psr_wait_for_idle_locked(new_crtc_state); > > local_irq_disable(); > > @@ -683,6 +685,8 @@ void intel_pipe_update_end(struct > intel_crtc_state *new_crtc_state) > > local_irq_enable(); > > + intel_psr_unlock(new_crtc_state); > + > if (intel_vgpu_active(dev_priv)) > return; > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > b/drivers/gpu/drm/i915/display/intel_psr.c > index 2da2468f555ec..58597480054eb 100644 > --- a/drivers/gpu/drm/i915/display/intel_psr.c > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > @@ -1548,10 +1548,19 @@ void > intel_psr2_program_plane_sel_fetch(struct intel_plane *plane, > void intel_psr2_program_trans_man_trk_ctl(const struct > intel_crtc_state *crtc_state) > { > struct drm_i915_private *dev_priv = to_i915(crtc_state- > >uapi.crtc->dev); > + struct intel_encoder *encoder; > > if (!crtc_state->enable_psr2_sel_fetch) > return; > > + for_each_intel_encoder_mask_with_psr(&dev_priv->drm, encoder, > + crtc_state- > >uapi.encoder_mask) { > + struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > + > + lockdep_assert_held(&intel_dp->psr.lock); > + break; > + } > + > intel_de_write(dev_priv, PSR2_MAN_TRK_CTL(crtc_state- > >cpu_transcoder), > crtc_state->psr2_man_track_ctl); > } > @@ -1919,13 +1928,13 @@ static int > _psr1_ready_for_pipe_update_locked(struct intel_dp *intel_dp) > } > > /** > - * intel_psr_wait_for_idle - wait for PSR be ready for a pipe update > + * intel_psr_wait_for_idle_locked - wait for PSR be ready for a pipe > update > * @new_crtc_state: new CRTC state > * > * This function is expected to be called from pipe_update_start() > where it is > * not expected to race with PSR enable or disable. > */ > -void intel_psr_wait_for_idle(const struct intel_crtc_state > *new_crtc_state) > +void intel_psr_wait_for_idle_locked(const struct intel_crtc_state > *new_crtc_state) > { > struct drm_i915_private *dev_priv = to_i915(new_crtc_state- > >uapi.crtc->dev); > struct intel_encoder *encoder; > @@ -1938,12 +1947,10 @@ void intel_psr_wait_for_idle(const struct > intel_crtc_state *new_crtc_state) > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > int ret; > > - mutex_lock(&intel_dp->psr.lock); > + lockdep_assert_held(&intel_dp->psr.lock); > > - if (!intel_dp->psr.enabled) { > - mutex_unlock(&intel_dp->psr.lock); > + if (!intel_dp->psr.enabled) > continue; > - } > > if (intel_dp->psr.psr2_enabled) > ret = > _psr2_ready_for_pipe_update_locked(intel_dp); > @@ -1952,8 +1959,6 @@ void intel_psr_wait_for_idle(const struct > intel_crtc_state *new_crtc_state) > > if (ret) > drm_err(&dev_priv->drm, "PSR wait timed out, > atomic update may fail\n"); > - > - mutex_unlock(&intel_dp->psr.lock); > } > } > > @@ -2444,3 +2449,51 @@ bool intel_psr_enabled(struct intel_dp > *intel_dp) > > return ret; > } > + > +/** > + * intel_psr_lock - grab psr.lock mutex > + * @crtc_state: the crtc state > + * > + * This is initially meant to be used by around CRTC update, when > + * vblank sensitive registers are updated and we need grab the lock > + * before it to avoid vblank evasion. > + */ > +void intel_psr_lock(const struct intel_crtc_state *crtc_state) > +{ > + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc- > >dev); > + struct intel_encoder *encoder; > + > + if (!crtc_state->has_psr) > + return; > + > + for_each_intel_encoder_mask_with_psr(&i915->drm, encoder, > + crtc_state- > >uapi.encoder_mask) { > + struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > + > + mutex_lock(&intel_dp->psr.lock); > + break; > + } > +} > + > +/** > + * intel_psr_lock - grab psr.lock mutex > + * @crtc_state: the crtc state > + * > + * Release the PSR lock that was held during pipe update. > + */ > +void intel_psr_unlock(const struct intel_crtc_state *crtc_state) > +{ > + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc- > >dev); > + struct intel_encoder *encoder; > + > + if (!crtc_state->has_psr) > + return; > + > + for_each_intel_encoder_mask_with_psr(&i915->drm, encoder, > + crtc_state- > >uapi.encoder_mask) { > + struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > + > + mutex_unlock(&intel_dp->psr.lock); > + break; > + } > +} > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h > b/drivers/gpu/drm/i915/display/intel_psr.h > index f6526d9ccfdc6..2ac3a46cccc50 100644 > --- a/drivers/gpu/drm/i915/display/intel_psr.h > +++ b/drivers/gpu/drm/i915/display/intel_psr.h > @@ -41,7 +41,7 @@ void intel_psr_get_config(struct intel_encoder > *encoder, > struct intel_crtc_state *pipe_config); > void intel_psr_irq_handler(struct intel_dp *intel_dp, u32 psr_iir); > void intel_psr_short_pulse(struct intel_dp *intel_dp); > -void intel_psr_wait_for_idle(const struct intel_crtc_state > *new_crtc_state); > +void intel_psr_wait_for_idle_locked(const struct intel_crtc_state > *new_crtc_state); > bool intel_psr_enabled(struct intel_dp *intel_dp); > int intel_psr2_sel_fetch_update(struct intel_atomic_state *state, > struct intel_crtc *crtc); > @@ -55,4 +55,7 @@ void intel_psr2_disable_plane_sel_fetch(struct > intel_plane *plane, > void intel_psr_pause(struct intel_dp *intel_dp); > void intel_psr_resume(struct intel_dp *intel_dp); > > +void intel_psr_lock(const struct intel_crtc_state *crtc_state); > +void intel_psr_unlock(const struct intel_crtc_state *crtc_state); > + > #endif /* __INTEL_PSR_H__ */
On Mon, 2022-04-04 at 07:41 +0000, Hogander, Jouni wrote: > Hello, > > Couple of questions below. > On Fri, 2022-04-01 at 15:29 -0700, José Roberto de Souza wrote: > > Frontbuffer rendering and page flips can race with each other > > and this can potentialy cause issues with PSR2 selective fetch. > > > > And because pipe/crtc updates are time sentive we can't grab the > > PSR lock after intel_pipe_update_start() and before > > intel_pipe_update_end(). > > > > So here adding the lock and unlock functions and calls, the > > proper PSR2 selective fetch handling will come in a separated patch. > > > > Have you ensured that there is no case where pipe_update_end is not > called? Yep. > > Why did you choose to add new hooks instead of using existing > intel_psr_pre_plane_update and intel_psr_post_plane_update? The lock would be held for too much time we those functions were used. In a modeset case it would take the lock before disable CRTC and only would release a way after pipes are enabled. If in the mean time something needs the PSR lock it would be blocked for a few milliseconds while here would be very small window of time. > > > Cc: Jouni Högander <jouni.hogander@intel.com> > > Cc: Mika Kahola <mika.kahola@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_crtc.c | 6 +- > > drivers/gpu/drm/i915/display/intel_psr.c | 69 ++++++++++++++++++++- > > -- > > drivers/gpu/drm/i915/display/intel_psr.h | 5 +- > > 3 files changed, 70 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c > > b/drivers/gpu/drm/i915/display/intel_crtc.c > > index f655c16228776..a5439182d5ae4 100644 > > --- a/drivers/gpu/drm/i915/display/intel_crtc.c > > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c > > @@ -507,6 +507,8 @@ void intel_pipe_update_start(struct > > intel_crtc_state *new_crtc_state) > > VBLANK_EVASION_TI > > ME_US); > > max = vblank_start - 1; > > > > + intel_psr_lock(new_crtc_state); > > + > > if (min <= 0 || max <= 0) > > goto irq_disable; > > > > @@ -518,7 +520,7 @@ void intel_pipe_update_start(struct > > intel_crtc_state *new_crtc_state) > > * VBL interrupts will start the PSR exit and prevent a PSR > > * re-entry as well. > > */ > > - intel_psr_wait_for_idle(new_crtc_state); > > + intel_psr_wait_for_idle_locked(new_crtc_state); > > > > local_irq_disable(); > > > > @@ -683,6 +685,8 @@ void intel_pipe_update_end(struct > > intel_crtc_state *new_crtc_state) > > > > local_irq_enable(); > > > > + intel_psr_unlock(new_crtc_state); > > + > > if (intel_vgpu_active(dev_priv)) > > return; > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > b/drivers/gpu/drm/i915/display/intel_psr.c > > index 2da2468f555ec..58597480054eb 100644 > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > @@ -1548,10 +1548,19 @@ void > > intel_psr2_program_plane_sel_fetch(struct intel_plane *plane, > > void intel_psr2_program_trans_man_trk_ctl(const struct > > intel_crtc_state *crtc_state) > > { > > struct drm_i915_private *dev_priv = to_i915(crtc_state- > > > uapi.crtc->dev); > > + struct intel_encoder *encoder; > > > > if (!crtc_state->enable_psr2_sel_fetch) > > return; > > > > + for_each_intel_encoder_mask_with_psr(&dev_priv->drm, encoder, > > + crtc_state- > > > uapi.encoder_mask) { > > + struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > + > > + lockdep_assert_held(&intel_dp->psr.lock); > > + break; > > + } > > + > > intel_de_write(dev_priv, PSR2_MAN_TRK_CTL(crtc_state- > > > cpu_transcoder), > > crtc_state->psr2_man_track_ctl); > > } > > @@ -1919,13 +1928,13 @@ static int > > _psr1_ready_for_pipe_update_locked(struct intel_dp *intel_dp) > > } > > > > /** > > - * intel_psr_wait_for_idle - wait for PSR be ready for a pipe update > > + * intel_psr_wait_for_idle_locked - wait for PSR be ready for a pipe > > update > > * @new_crtc_state: new CRTC state > > * > > * This function is expected to be called from pipe_update_start() > > where it is > > * not expected to race with PSR enable or disable. > > */ > > -void intel_psr_wait_for_idle(const struct intel_crtc_state > > *new_crtc_state) > > +void intel_psr_wait_for_idle_locked(const struct intel_crtc_state > > *new_crtc_state) > > { > > struct drm_i915_private *dev_priv = to_i915(new_crtc_state- > > > uapi.crtc->dev); > > struct intel_encoder *encoder; > > @@ -1938,12 +1947,10 @@ void intel_psr_wait_for_idle(const struct > > intel_crtc_state *new_crtc_state) > > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > int ret; > > > > - mutex_lock(&intel_dp->psr.lock); > > + lockdep_assert_held(&intel_dp->psr.lock); > > > > - if (!intel_dp->psr.enabled) { > > - mutex_unlock(&intel_dp->psr.lock); > > + if (!intel_dp->psr.enabled) > > continue; > > - } > > > > if (intel_dp->psr.psr2_enabled) > > ret = > > _psr2_ready_for_pipe_update_locked(intel_dp); > > @@ -1952,8 +1959,6 @@ void intel_psr_wait_for_idle(const struct > > intel_crtc_state *new_crtc_state) > > > > if (ret) > > drm_err(&dev_priv->drm, "PSR wait timed out, > > atomic update may fail\n"); > > - > > - mutex_unlock(&intel_dp->psr.lock); > > } > > } > > > > @@ -2444,3 +2449,51 @@ bool intel_psr_enabled(struct intel_dp > > *intel_dp) > > > > return ret; > > } > > + > > +/** > > + * intel_psr_lock - grab psr.lock mutex > > + * @crtc_state: the crtc state > > + * > > + * This is initially meant to be used by around CRTC update, when > > + * vblank sensitive registers are updated and we need grab the lock > > + * before it to avoid vblank evasion. > > + */ > > +void intel_psr_lock(const struct intel_crtc_state *crtc_state) > > +{ > > + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc- > > > dev); > > + struct intel_encoder *encoder; > > + > > + if (!crtc_state->has_psr) > > + return; > > + > > + for_each_intel_encoder_mask_with_psr(&i915->drm, encoder, > > + crtc_state- > > > uapi.encoder_mask) { > > + struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > + > > + mutex_lock(&intel_dp->psr.lock); > > + break; > > + } > > +} > > + > > +/** > > + * intel_psr_lock - grab psr.lock mutex > > + * @crtc_state: the crtc state > > + * > > + * Release the PSR lock that was held during pipe update. > > + */ > > +void intel_psr_unlock(const struct intel_crtc_state *crtc_state) > > +{ > > + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc- > > > dev); > > + struct intel_encoder *encoder; > > + > > + if (!crtc_state->has_psr) > > + return; > > + > > + for_each_intel_encoder_mask_with_psr(&i915->drm, encoder, > > + crtc_state- > > > uapi.encoder_mask) { > > + struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > + > > + mutex_unlock(&intel_dp->psr.lock); > > + break; > > + } > > +} > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h > > b/drivers/gpu/drm/i915/display/intel_psr.h > > index f6526d9ccfdc6..2ac3a46cccc50 100644 > > --- a/drivers/gpu/drm/i915/display/intel_psr.h > > +++ b/drivers/gpu/drm/i915/display/intel_psr.h > > @@ -41,7 +41,7 @@ void intel_psr_get_config(struct intel_encoder > > *encoder, > > struct intel_crtc_state *pipe_config); > > void intel_psr_irq_handler(struct intel_dp *intel_dp, u32 psr_iir); > > void intel_psr_short_pulse(struct intel_dp *intel_dp); > > -void intel_psr_wait_for_idle(const struct intel_crtc_state > > *new_crtc_state); > > +void intel_psr_wait_for_idle_locked(const struct intel_crtc_state > > *new_crtc_state); > > bool intel_psr_enabled(struct intel_dp *intel_dp); > > int intel_psr2_sel_fetch_update(struct intel_atomic_state *state, > > struct intel_crtc *crtc); > > @@ -55,4 +55,7 @@ void intel_psr2_disable_plane_sel_fetch(struct > > intel_plane *plane, > > void intel_psr_pause(struct intel_dp *intel_dp); > > void intel_psr_resume(struct intel_dp *intel_dp); > > > > +void intel_psr_lock(const struct intel_crtc_state *crtc_state); > > +void intel_psr_unlock(const struct intel_crtc_state *crtc_state); > > + > > #endif /* __INTEL_PSR_H__ */ >
On Mon, 2022-04-04 at 13:43 +0000, Souza, Jose wrote: > On Mon, 2022-04-04 at 07:41 +0000, Hogander, Jouni wrote: > > Hello, > > > > Couple of questions below. > > On Fri, 2022-04-01 at 15:29 -0700, José Roberto de Souza wrote: > > > Frontbuffer rendering and page flips can race with each other > > > and this can potentialy cause issues with PSR2 selective fetch. > > > > > > And because pipe/crtc updates are time sentive we can't grab the > > > PSR lock after intel_pipe_update_start() and before > > > intel_pipe_update_end(). > > > > > > So here adding the lock and unlock functions and calls, the > > > proper PSR2 selective fetch handling will come in a separated > > > patch. > > > > > > > Have you ensured that there is no case where pipe_update_end is not > > called? > > Yep. > > > Why did you choose to add new hooks instead of using existing > > intel_psr_pre_plane_update and intel_psr_post_plane_update? > > The lock would be held for too much time we those functions were > used. > In a modeset case it would take the lock before disable CRTC and only > would release a way after pipes are enabled. > > If in the mean time something needs the PSR lock it would be blocked > for a few milliseconds while here would be very small window of time. Reviewed-by: Jouni Högander <jouni.hogander@intel.com> > > > > Cc: Jouni Högander <jouni.hogander@intel.com> > > > Cc: Mika Kahola <mika.kahola@intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_crtc.c | 6 +- > > > drivers/gpu/drm/i915/display/intel_psr.c | 69 > > > ++++++++++++++++++++- > > > -- > > > drivers/gpu/drm/i915/display/intel_psr.h | 5 +- > > > 3 files changed, 70 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c > > > b/drivers/gpu/drm/i915/display/intel_crtc.c > > > index f655c16228776..a5439182d5ae4 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_crtc.c > > > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c > > > @@ -507,6 +507,8 @@ void intel_pipe_update_start(struct > > > intel_crtc_state *new_crtc_state) > > > VBLANK_EVASIO > > > N_TI > > > ME_US); > > > max = vblank_start - 1; > > > > > > + intel_psr_lock(new_crtc_state); > > > + > > > if (min <= 0 || max <= 0) > > > goto irq_disable; > > > > > > @@ -518,7 +520,7 @@ void intel_pipe_update_start(struct > > > intel_crtc_state *new_crtc_state) > > > * VBL interrupts will start the PSR exit and prevent a PSR > > > * re-entry as well. > > > */ > > > - intel_psr_wait_for_idle(new_crtc_state); > > > + intel_psr_wait_for_idle_locked(new_crtc_state); > > > > > > local_irq_disable(); > > > > > > @@ -683,6 +685,8 @@ void intel_pipe_update_end(struct > > > intel_crtc_state *new_crtc_state) > > > > > > local_irq_enable(); > > > > > > + intel_psr_unlock(new_crtc_state); > > > + > > > if (intel_vgpu_active(dev_priv)) > > > return; > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > > b/drivers/gpu/drm/i915/display/intel_psr.c > > > index 2da2468f555ec..58597480054eb 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > @@ -1548,10 +1548,19 @@ void > > > intel_psr2_program_plane_sel_fetch(struct intel_plane *plane, > > > void intel_psr2_program_trans_man_trk_ctl(const struct > > > intel_crtc_state *crtc_state) > > > { > > > struct drm_i915_private *dev_priv = to_i915(crtc_state- > > > > uapi.crtc->dev); > > > + struct intel_encoder *encoder; > > > > > > if (!crtc_state->enable_psr2_sel_fetch) > > > return; > > > > > > + for_each_intel_encoder_mask_with_psr(&dev_priv->drm, > > > encoder, > > > + crtc_state- > > > > uapi.encoder_mask) { > > > + struct intel_dp *intel_dp = > > > enc_to_intel_dp(encoder); > > > + > > > + lockdep_assert_held(&intel_dp->psr.lock); > > > + break; > > > + } > > > + > > > intel_de_write(dev_priv, PSR2_MAN_TRK_CTL(crtc_state- > > > > cpu_transcoder), > > > crtc_state->psr2_man_track_ctl); > > > } > > > @@ -1919,13 +1928,13 @@ static int > > > _psr1_ready_for_pipe_update_locked(struct intel_dp *intel_dp) > > > } > > > > > > /** > > > - * intel_psr_wait_for_idle - wait for PSR be ready for a pipe > > > update > > > + * intel_psr_wait_for_idle_locked - wait for PSR be ready for a > > > pipe > > > update > > > * @new_crtc_state: new CRTC state > > > * > > > * This function is expected to be called from > > > pipe_update_start() > > > where it is > > > * not expected to race with PSR enable or disable. > > > */ > > > -void intel_psr_wait_for_idle(const struct intel_crtc_state > > > *new_crtc_state) > > > +void intel_psr_wait_for_idle_locked(const struct > > > intel_crtc_state > > > *new_crtc_state) > > > { > > > struct drm_i915_private *dev_priv = to_i915(new_crtc_state- > > > > uapi.crtc->dev); > > > struct intel_encoder *encoder; > > > @@ -1938,12 +1947,10 @@ void intel_psr_wait_for_idle(const struct > > > intel_crtc_state *new_crtc_state) > > > struct intel_dp *intel_dp = > > > enc_to_intel_dp(encoder); > > > int ret; > > > > > > - mutex_lock(&intel_dp->psr.lock); > > > + lockdep_assert_held(&intel_dp->psr.lock); > > > > > > - if (!intel_dp->psr.enabled) { > > > - mutex_unlock(&intel_dp->psr.lock); > > > + if (!intel_dp->psr.enabled) > > > continue; > > > - } > > > > > > if (intel_dp->psr.psr2_enabled) > > > ret = > > > _psr2_ready_for_pipe_update_locked(intel_dp); > > > @@ -1952,8 +1959,6 @@ void intel_psr_wait_for_idle(const struct > > > intel_crtc_state *new_crtc_state) > > > > > > if (ret) > > > drm_err(&dev_priv->drm, "PSR wait timed > > > out, > > > atomic update may fail\n"); > > > - > > > - mutex_unlock(&intel_dp->psr.lock); > > > } > > > } > > > > > > @@ -2444,3 +2449,51 @@ bool intel_psr_enabled(struct intel_dp > > > *intel_dp) > > > > > > return ret; > > > } > > > + > > > +/** > > > + * intel_psr_lock - grab psr.lock mutex > > > + * @crtc_state: the crtc state > > > + * > > > + * This is initially meant to be used by around CRTC update, > > > when > > > + * vblank sensitive registers are updated and we need grab the > > > lock > > > + * before it to avoid vblank evasion. > > > + */ > > > +void intel_psr_lock(const struct intel_crtc_state *crtc_state) > > > +{ > > > + struct drm_i915_private *i915 = to_i915(crtc_state- > > > >uapi.crtc- > > > > dev); > > > + struct intel_encoder *encoder; > > > + > > > + if (!crtc_state->has_psr) > > > + return; > > > + > > > + for_each_intel_encoder_mask_with_psr(&i915->drm, encoder, > > > + crtc_state- > > > > uapi.encoder_mask) { > > > + struct intel_dp *intel_dp = > > > enc_to_intel_dp(encoder); > > > + > > > + mutex_lock(&intel_dp->psr.lock); > > > + break; > > > + } > > > +} > > > + > > > +/** > > > + * intel_psr_lock - grab psr.lock mutex > > > + * @crtc_state: the crtc state > > > + * > > > + * Release the PSR lock that was held during pipe update. > > > + */ > > > +void intel_psr_unlock(const struct intel_crtc_state *crtc_state) > > > +{ > > > + struct drm_i915_private *i915 = to_i915(crtc_state- > > > >uapi.crtc- > > > > dev); > > > + struct intel_encoder *encoder; > > > + > > > + if (!crtc_state->has_psr) > > > + return; > > > + > > > + for_each_intel_encoder_mask_with_psr(&i915->drm, encoder, > > > + crtc_state- > > > > uapi.encoder_mask) { > > > + struct intel_dp *intel_dp = > > > enc_to_intel_dp(encoder); > > > + > > > + mutex_unlock(&intel_dp->psr.lock); > > > + break; > > > + } > > > +} > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h > > > b/drivers/gpu/drm/i915/display/intel_psr.h > > > index f6526d9ccfdc6..2ac3a46cccc50 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_psr.h > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.h > > > @@ -41,7 +41,7 @@ void intel_psr_get_config(struct intel_encoder > > > *encoder, > > > struct intel_crtc_state *pipe_config); > > > void intel_psr_irq_handler(struct intel_dp *intel_dp, u32 > > > psr_iir); > > > void intel_psr_short_pulse(struct intel_dp *intel_dp); > > > -void intel_psr_wait_for_idle(const struct intel_crtc_state > > > *new_crtc_state); > > > +void intel_psr_wait_for_idle_locked(const struct > > > intel_crtc_state > > > *new_crtc_state); > > > bool intel_psr_enabled(struct intel_dp *intel_dp); > > > int intel_psr2_sel_fetch_update(struct intel_atomic_state > > > *state, > > > struct intel_crtc *crtc); > > > @@ -55,4 +55,7 @@ void intel_psr2_disable_plane_sel_fetch(struct > > > intel_plane *plane, > > > void intel_psr_pause(struct intel_dp *intel_dp); > > > void intel_psr_resume(struct intel_dp *intel_dp); > > > > > > +void intel_psr_lock(const struct intel_crtc_state *crtc_state); > > > +void intel_psr_unlock(const struct intel_crtc_state > > > *crtc_state); > > > + > > > #endif /* __INTEL_PSR_H__ */
diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c index f655c16228776..a5439182d5ae4 100644 --- a/drivers/gpu/drm/i915/display/intel_crtc.c +++ b/drivers/gpu/drm/i915/display/intel_crtc.c @@ -507,6 +507,8 @@ void intel_pipe_update_start(struct intel_crtc_state *new_crtc_state) VBLANK_EVASION_TIME_US); max = vblank_start - 1; + intel_psr_lock(new_crtc_state); + if (min <= 0 || max <= 0) goto irq_disable; @@ -518,7 +520,7 @@ void intel_pipe_update_start(struct intel_crtc_state *new_crtc_state) * VBL interrupts will start the PSR exit and prevent a PSR * re-entry as well. */ - intel_psr_wait_for_idle(new_crtc_state); + intel_psr_wait_for_idle_locked(new_crtc_state); local_irq_disable(); @@ -683,6 +685,8 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state) local_irq_enable(); + intel_psr_unlock(new_crtc_state); + if (intel_vgpu_active(dev_priv)) return; diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c index 2da2468f555ec..58597480054eb 100644 --- a/drivers/gpu/drm/i915/display/intel_psr.c +++ b/drivers/gpu/drm/i915/display/intel_psr.c @@ -1548,10 +1548,19 @@ void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane, void intel_psr2_program_trans_man_trk_ctl(const struct intel_crtc_state *crtc_state) { struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev); + struct intel_encoder *encoder; if (!crtc_state->enable_psr2_sel_fetch) return; + for_each_intel_encoder_mask_with_psr(&dev_priv->drm, encoder, + crtc_state->uapi.encoder_mask) { + struct intel_dp *intel_dp = enc_to_intel_dp(encoder); + + lockdep_assert_held(&intel_dp->psr.lock); + break; + } + intel_de_write(dev_priv, PSR2_MAN_TRK_CTL(crtc_state->cpu_transcoder), crtc_state->psr2_man_track_ctl); } @@ -1919,13 +1928,13 @@ static int _psr1_ready_for_pipe_update_locked(struct intel_dp *intel_dp) } /** - * intel_psr_wait_for_idle - wait for PSR be ready for a pipe update + * intel_psr_wait_for_idle_locked - wait for PSR be ready for a pipe update * @new_crtc_state: new CRTC state * * This function is expected to be called from pipe_update_start() where it is * not expected to race with PSR enable or disable. */ -void intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state) +void intel_psr_wait_for_idle_locked(const struct intel_crtc_state *new_crtc_state) { struct drm_i915_private *dev_priv = to_i915(new_crtc_state->uapi.crtc->dev); struct intel_encoder *encoder; @@ -1938,12 +1947,10 @@ void intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state) struct intel_dp *intel_dp = enc_to_intel_dp(encoder); int ret; - mutex_lock(&intel_dp->psr.lock); + lockdep_assert_held(&intel_dp->psr.lock); - if (!intel_dp->psr.enabled) { - mutex_unlock(&intel_dp->psr.lock); + if (!intel_dp->psr.enabled) continue; - } if (intel_dp->psr.psr2_enabled) ret = _psr2_ready_for_pipe_update_locked(intel_dp); @@ -1952,8 +1959,6 @@ void intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state) if (ret) drm_err(&dev_priv->drm, "PSR wait timed out, atomic update may fail\n"); - - mutex_unlock(&intel_dp->psr.lock); } } @@ -2444,3 +2449,51 @@ bool intel_psr_enabled(struct intel_dp *intel_dp) return ret; } + +/** + * intel_psr_lock - grab psr.lock mutex + * @crtc_state: the crtc state + * + * This is initially meant to be used by around CRTC update, when + * vblank sensitive registers are updated and we need grab the lock + * before it to avoid vblank evasion. + */ +void intel_psr_lock(const struct intel_crtc_state *crtc_state) +{ + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev); + struct intel_encoder *encoder; + + if (!crtc_state->has_psr) + return; + + for_each_intel_encoder_mask_with_psr(&i915->drm, encoder, + crtc_state->uapi.encoder_mask) { + struct intel_dp *intel_dp = enc_to_intel_dp(encoder); + + mutex_lock(&intel_dp->psr.lock); + break; + } +} + +/** + * intel_psr_lock - grab psr.lock mutex + * @crtc_state: the crtc state + * + * Release the PSR lock that was held during pipe update. + */ +void intel_psr_unlock(const struct intel_crtc_state *crtc_state) +{ + struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev); + struct intel_encoder *encoder; + + if (!crtc_state->has_psr) + return; + + for_each_intel_encoder_mask_with_psr(&i915->drm, encoder, + crtc_state->uapi.encoder_mask) { + struct intel_dp *intel_dp = enc_to_intel_dp(encoder); + + mutex_unlock(&intel_dp->psr.lock); + break; + } +} diff --git a/drivers/gpu/drm/i915/display/intel_psr.h b/drivers/gpu/drm/i915/display/intel_psr.h index f6526d9ccfdc6..2ac3a46cccc50 100644 --- a/drivers/gpu/drm/i915/display/intel_psr.h +++ b/drivers/gpu/drm/i915/display/intel_psr.h @@ -41,7 +41,7 @@ void intel_psr_get_config(struct intel_encoder *encoder, struct intel_crtc_state *pipe_config); void intel_psr_irq_handler(struct intel_dp *intel_dp, u32 psr_iir); void intel_psr_short_pulse(struct intel_dp *intel_dp); -void intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state); +void intel_psr_wait_for_idle_locked(const struct intel_crtc_state *new_crtc_state); bool intel_psr_enabled(struct intel_dp *intel_dp); int intel_psr2_sel_fetch_update(struct intel_atomic_state *state, struct intel_crtc *crtc); @@ -55,4 +55,7 @@ void intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane, void intel_psr_pause(struct intel_dp *intel_dp); void intel_psr_resume(struct intel_dp *intel_dp); +void intel_psr_lock(const struct intel_crtc_state *crtc_state); +void intel_psr_unlock(const struct intel_crtc_state *crtc_state); + #endif /* __INTEL_PSR_H__ */
Frontbuffer rendering and page flips can race with each other and this can potentialy cause issues with PSR2 selective fetch. And because pipe/crtc updates are time sentive we can't grab the PSR lock after intel_pipe_update_start() and before intel_pipe_update_end(). So here adding the lock and unlock functions and calls, the proper PSR2 selective fetch handling will come in a separated patch. Cc: Jouni Högander <jouni.hogander@intel.com> Cc: Mika Kahola <mika.kahola@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/display/intel_crtc.c | 6 +- drivers/gpu/drm/i915/display/intel_psr.c | 69 ++++++++++++++++++++--- drivers/gpu/drm/i915/display/intel_psr.h | 5 +- 3 files changed, 70 insertions(+), 10 deletions(-)