Message ID | 1655808800-3996-6-git-send-email-quic_vpolimer@quicinc.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v1] drm/msm/dpu: consider vertical front porch in the prefill bw calculation | expand |
On 21/06/2022 13:53, Vinod Polimera wrote: > Enable PSR on eDP interface using drm self-refresh librabry. > This patch uses a trigger from self-refresh library to enter/exit > into PSR, when there are no updates from framework. > > Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com> > Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 36 ++++++++++++++++++++++++----- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 20 +++++++++++++++- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +- > 3 files changed, 50 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > index b56f777..c6e4f03 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > @@ -18,6 +18,7 @@ > #include <drm/drm_probe_helper.h> > #include <drm/drm_rect.h> > #include <drm/drm_vblank.h> > +#include <drm/drm_self_refresh_helper.h> > > #include "dpu_kms.h" > #include "dpu_hw_lm.h" > @@ -955,24 +956,39 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, > crtc); > struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); > struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state); > - struct drm_encoder *encoder; > + struct drm_encoder *encoder = NULL; > unsigned long flags; > bool release_bandwidth = false; > > DRM_DEBUG_KMS("crtc%d\n", crtc->base.id); > > + if (old_crtc_state->self_refresh_active) { > + drm_for_each_encoder_mask(encoder, crtc->dev, > + old_crtc_state->encoder_mask) { > + dpu_encoder_assign_crtc(encoder, NULL); I think we should drop dpu_encoder_assign_crtc completely and use your new helpers instead. Having to manually duplicate existing link sounds like a bad idea. > + } > + return; > + } > + > /* Disable/save vblank irq handling */ > drm_crtc_vblank_off(crtc); > > drm_for_each_encoder_mask(encoder, crtc->dev, > old_crtc_state->encoder_mask) { > - /* in video mode, we hold an extra bandwidth reference > + /* > + * in video mode, we hold an extra bandwidth reference Unrelated to this patch. > * as we cannot drop bandwidth at frame-done if any > * crtc is being used in video mode. > */ > if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_VIDEO) > release_bandwidth = true; > - dpu_encoder_assign_crtc(encoder, NULL); > + /* > + * If disable is triggered during psr active(e.g: screen dim in PSR), > + * we will need encoder->crtc connection to process the device sleep & > + * preserve it during psr sequence. > + */ > + if (!crtc->state->self_refresh_active) > + dpu_encoder_assign_crtc(encoder, NULL); > } > > /* wait for frame_event_done completion */ > @@ -1020,7 +1036,9 @@ static void dpu_crtc_enable(struct drm_crtc *crtc, > struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); > struct drm_encoder *encoder; > bool request_bandwidth = false; > + struct drm_crtc_state *old_crtc_state; > > + old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc); > pm_runtime_get_sync(crtc->dev->dev); > > DRM_DEBUG_KMS("crtc%d\n", crtc->base.id); > @@ -1042,8 +1060,9 @@ static void dpu_crtc_enable(struct drm_crtc *crtc, > trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc); > dpu_crtc->enabled = true; > > - drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) > - dpu_encoder_assign_crtc(encoder, crtc); > + if (!old_crtc_state->self_refresh_active) > + drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) > + dpu_encoder_assign_crtc(encoder, crtc); > > /* Enable/restore vblank irq handling */ > drm_crtc_vblank_on(crtc); > @@ -1525,7 +1544,7 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane, > { > struct drm_crtc *crtc = NULL; > struct dpu_crtc *dpu_crtc = NULL; > - int i; > + int i, ret; > > dpu_crtc = kzalloc(sizeof(*dpu_crtc), GFP_KERNEL); > if (!dpu_crtc) > @@ -1562,6 +1581,11 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane, > /* initialize event handling */ > spin_lock_init(&dpu_crtc->event_lock); > > + ret = drm_self_refresh_helper_init(crtc); > + if (ret) > + DPU_ERROR("Failed to initialize %s with self-refresh helpers %d\n", > + crtc->name, ret); > + > DRM_DEBUG_KMS("%s: successfully initialized crtc\n", dpu_crtc->name); > return crtc; > } > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index cc2809b..234e95d 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -225,6 +225,11 @@ bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc) > return dpu_enc->wide_bus_en; > } > > +static inline bool is_self_refresh_active(const struct drm_crtc_state *state) > +{ > + return (state && state->self_refresh_active); > +} > + > static void _dpu_encoder_setup_dither(struct dpu_hw_pingpong *hw_pp, unsigned bpc) > { > struct dpu_hw_dither_cfg dither_cfg = { 0 }; > @@ -592,7 +597,8 @@ static int dpu_encoder_virt_atomic_check( > if (drm_atomic_crtc_needs_modeset(crtc_state)) { > dpu_rm_release(global_state, drm_enc); > > - if (!crtc_state->active_changed || crtc_state->active) > + if (!crtc_state->active_changed || crtc_state->active || > + crtc_state->self_refresh_active) This condition should be changed to use enabled rather than active. Quoting KMS documentation: 'The driver must not release any shared resources if active is set to false but enable still true...' > ret = dpu_rm_reserve(&dpu_kms->rm, global_state, > drm_enc, crtc_state, topology); > } > @@ -1171,11 +1177,23 @@ static void dpu_encoder_virt_atomic_disable(struct drm_encoder *drm_enc, > struct drm_atomic_state *state) > { > struct dpu_encoder_virt *dpu_enc = NULL; > + struct drm_crtc *crtc; > + struct drm_crtc_state *old_state; > int i = 0; > > dpu_enc = to_dpu_encoder_virt(drm_enc); > DPU_DEBUG_ENC(dpu_enc, "\n"); > > + crtc = dpu_enc->crtc; > + old_state = drm_atomic_get_old_crtc_state(state, crtc); > + > + /* > + * The encoder disabled already occurred when self refresh mode > + * was set earlier, in the old_state for the corresponding crtc. > + */ > + if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && is_self_refresh_active(old_state)) > + return; > + Why do you need to check the encoder_type? > mutex_lock(&dpu_enc->enc_lock); > dpu_enc->enabled = false; > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > index bce4764..cc0a674 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > @@ -507,7 +507,7 @@ static void dpu_kms_wait_for_commit_done(struct msm_kms *kms, > return; > } > > - if (!crtc->state->active) { > + if (!drm_atomic_crtc_effectively_active(crtc->state)) { > DPU_DEBUG("[crtc:%d] not active\n", crtc->base.id); > return; > }
Hi Vinod,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm/drm-next]
[also build test WARNING on drm-exynos/exynos-drm-next drm-tip/drm-tip tegra-drm/drm/tegra/for-next linus/master v5.19-rc3 next-20220622]
[cannot apply to drm-intel/for-linux-next airlied/drm-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/Vinod-Polimera/drm-msm-dp-Add-basic-PSR-support-for-eDP/20220621-195406
base: git://anongit.freedesktop.org/drm/drm drm-next
config: arm64-randconfig-r025-20220622 (https://download.01.org/0day-ci/archive/20220623/202206230551.H0oXeV2E-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 8b8d126598ce7bd5243da7f94f69fa1104288bee)
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
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/2c3c31343481a4faf2402cf513c85fb7d75ce205
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Vinod-Polimera/drm-msm-dp-Add-basic-PSR-support-for-eDP/20220621-195406
git checkout 2c3c31343481a4faf2402cf513c85fb7d75ce205
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c:1064:3: warning: add explicit braces to avoid dangling else [-Wdangling-else]
drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
^
include/drm/drm_encoder.h:314:3: note: expanded from macro 'drm_for_each_encoder_mask'
for_each_if ((encoder_mask) & drm_encoder_mask(encoder))
^
include/drm/drm_util.h:63:53: note: expanded from macro 'for_each_if'
#define for_each_if(condition) if (!(condition)) {} else
^
1 warning generated.
vim +1064 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 1032
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 1033 static void dpu_crtc_enable(struct drm_crtc *crtc,
351f950db4ab28 Maxime Ripard 2020-10-08 1034 struct drm_atomic_state *state)
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 1035 {
e12e5263bf1d8d Rob Clark 2020-09-07 1036 struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 1037 struct drm_encoder *encoder;
35c719da95c0d2 Rob Clark 2020-08-11 1038 bool request_bandwidth = false;
2c3c31343481a4 Vinod Polimera 2022-06-21 1039 struct drm_crtc_state *old_crtc_state;
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 1040
2c3c31343481a4 Vinod Polimera 2022-06-21 1041 old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
b77d0f0d4ee757 Sean Paul 2018-11-16 1042 pm_runtime_get_sync(crtc->dev->dev);
b77d0f0d4ee757 Sean Paul 2018-11-16 1043
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 1044 DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 1045
241b507c166fef Rob Clark 2019-08-20 1046 drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) {
241b507c166fef Rob Clark 2019-08-20 1047 /* in video mode, we hold an extra bandwidth reference
241b507c166fef Rob Clark 2019-08-20 1048 * as we cannot drop bandwidth at frame-done if any
241b507c166fef Rob Clark 2019-08-20 1049 * crtc is being used in video mode.
241b507c166fef Rob Clark 2019-08-20 1050 */
241b507c166fef Rob Clark 2019-08-20 1051 if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_VIDEO)
241b507c166fef Rob Clark 2019-08-20 1052 request_bandwidth = true;
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 1053 dpu_encoder_register_frame_event_callback(encoder,
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 1054 dpu_crtc_frame_event_cb, (void *)crtc);
241b507c166fef Rob Clark 2019-08-20 1055 }
241b507c166fef Rob Clark 2019-08-20 1056
241b507c166fef Rob Clark 2019-08-20 1057 if (request_bandwidth)
241b507c166fef Rob Clark 2019-08-20 1058 atomic_inc(&_dpu_crtc_get_kms(crtc)->bandwidth_ref);
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 1059
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 1060 trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc);
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 1061 dpu_crtc->enabled = true;
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 1062
2c3c31343481a4 Vinod Polimera 2022-06-21 1063 if (!old_crtc_state->self_refresh_active)
a796ba2cb3dde3 Sean Paul 2018-11-16 @1064 drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
a796ba2cb3dde3 Sean Paul 2018-11-16 1065 dpu_encoder_assign_crtc(encoder, crtc);
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 1066
2f2eb723b50b4d Rajesh Yadav 2018-07-13 1067 /* Enable/restore vblank irq handling */
2f2eb723b50b4d Rajesh Yadav 2018-07-13 1068 drm_crtc_vblank_on(crtc);
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 1069 }
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 1070
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index b56f777..c6e4f03 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -18,6 +18,7 @@ #include <drm/drm_probe_helper.h> #include <drm/drm_rect.h> #include <drm/drm_vblank.h> +#include <drm/drm_self_refresh_helper.h> #include "dpu_kms.h" #include "dpu_hw_lm.h" @@ -955,24 +956,39 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, crtc); struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state); - struct drm_encoder *encoder; + struct drm_encoder *encoder = NULL; unsigned long flags; bool release_bandwidth = false; DRM_DEBUG_KMS("crtc%d\n", crtc->base.id); + if (old_crtc_state->self_refresh_active) { + drm_for_each_encoder_mask(encoder, crtc->dev, + old_crtc_state->encoder_mask) { + dpu_encoder_assign_crtc(encoder, NULL); + } + return; + } + /* Disable/save vblank irq handling */ drm_crtc_vblank_off(crtc); drm_for_each_encoder_mask(encoder, crtc->dev, old_crtc_state->encoder_mask) { - /* in video mode, we hold an extra bandwidth reference + /* + * in video mode, we hold an extra bandwidth reference * as we cannot drop bandwidth at frame-done if any * crtc is being used in video mode. */ if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_VIDEO) release_bandwidth = true; - dpu_encoder_assign_crtc(encoder, NULL); + /* + * If disable is triggered during psr active(e.g: screen dim in PSR), + * we will need encoder->crtc connection to process the device sleep & + * preserve it during psr sequence. + */ + if (!crtc->state->self_refresh_active) + dpu_encoder_assign_crtc(encoder, NULL); } /* wait for frame_event_done completion */ @@ -1020,7 +1036,9 @@ static void dpu_crtc_enable(struct drm_crtc *crtc, struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); struct drm_encoder *encoder; bool request_bandwidth = false; + struct drm_crtc_state *old_crtc_state; + old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc); pm_runtime_get_sync(crtc->dev->dev); DRM_DEBUG_KMS("crtc%d\n", crtc->base.id); @@ -1042,8 +1060,9 @@ static void dpu_crtc_enable(struct drm_crtc *crtc, trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc); dpu_crtc->enabled = true; - drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) - dpu_encoder_assign_crtc(encoder, crtc); + if (!old_crtc_state->self_refresh_active) + drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) + dpu_encoder_assign_crtc(encoder, crtc); /* Enable/restore vblank irq handling */ drm_crtc_vblank_on(crtc); @@ -1525,7 +1544,7 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane, { struct drm_crtc *crtc = NULL; struct dpu_crtc *dpu_crtc = NULL; - int i; + int i, ret; dpu_crtc = kzalloc(sizeof(*dpu_crtc), GFP_KERNEL); if (!dpu_crtc) @@ -1562,6 +1581,11 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane, /* initialize event handling */ spin_lock_init(&dpu_crtc->event_lock); + ret = drm_self_refresh_helper_init(crtc); + if (ret) + DPU_ERROR("Failed to initialize %s with self-refresh helpers %d\n", + crtc->name, ret); + DRM_DEBUG_KMS("%s: successfully initialized crtc\n", dpu_crtc->name); return crtc; } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index cc2809b..234e95d 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -225,6 +225,11 @@ bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc) return dpu_enc->wide_bus_en; } +static inline bool is_self_refresh_active(const struct drm_crtc_state *state) +{ + return (state && state->self_refresh_active); +} + static void _dpu_encoder_setup_dither(struct dpu_hw_pingpong *hw_pp, unsigned bpc) { struct dpu_hw_dither_cfg dither_cfg = { 0 }; @@ -592,7 +597,8 @@ static int dpu_encoder_virt_atomic_check( if (drm_atomic_crtc_needs_modeset(crtc_state)) { dpu_rm_release(global_state, drm_enc); - if (!crtc_state->active_changed || crtc_state->active) + if (!crtc_state->active_changed || crtc_state->active || + crtc_state->self_refresh_active) ret = dpu_rm_reserve(&dpu_kms->rm, global_state, drm_enc, crtc_state, topology); } @@ -1171,11 +1177,23 @@ static void dpu_encoder_virt_atomic_disable(struct drm_encoder *drm_enc, struct drm_atomic_state *state) { struct dpu_encoder_virt *dpu_enc = NULL; + struct drm_crtc *crtc; + struct drm_crtc_state *old_state; int i = 0; dpu_enc = to_dpu_encoder_virt(drm_enc); DPU_DEBUG_ENC(dpu_enc, "\n"); + crtc = dpu_enc->crtc; + old_state = drm_atomic_get_old_crtc_state(state, crtc); + + /* + * The encoder disabled already occurred when self refresh mode + * was set earlier, in the old_state for the corresponding crtc. + */ + if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && is_self_refresh_active(old_state)) + return; + mutex_lock(&dpu_enc->enc_lock); dpu_enc->enabled = false; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index bce4764..cc0a674 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -507,7 +507,7 @@ static void dpu_kms_wait_for_commit_done(struct msm_kms *kms, return; } - if (!crtc->state->active) { + if (!drm_atomic_crtc_effectively_active(crtc->state)) { DPU_DEBUG("[crtc:%d] not active\n", crtc->base.id); return; }