| Message ID | 20230921200312.3989073-3-l.stach@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/6] drm: lcdif: improve burst size configuration comment | expand |
Hi Lucas, kernel test robot noticed the following build warnings: [auto build test WARNING on drm-misc/drm-misc-next] [also build test WARNING on linus/master v6.6-rc2 next-20230921] [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#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Lucas-Stach/drm-lcdif-don-t-clear-unrelated-bits-in-CTRLDESCL0_5-when-setting-up-format/20230922-040438 base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next patch link: https://lore.kernel.org/r/20230921200312.3989073-3-l.stach%40pengutronix.de patch subject: [PATCH v2 3/6] drm: lcdif: rework runtime PM handling in the atomic commit config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230922/202309220530.84SlbdTU-lkp@intel.com/config) compiler: m68k-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230922/202309220530.84SlbdTU-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202309220530.84SlbdTU-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/gpu/drm/mxsfb/lcdif_drv.c:39:6: warning: no previous prototype for 'lcdif_commit_tail' [-Wmissing-prototypes] 39 | void lcdif_commit_tail(struct drm_atomic_state *old_state) | ^~~~~~~~~~~~~~~~~ vim +/lcdif_commit_tail +39 drivers/gpu/drm/mxsfb/lcdif_drv.c 38 > 39 void lcdif_commit_tail(struct drm_atomic_state *old_state) 40 { 41 struct drm_device *drm = old_state->dev; 42 43 pm_runtime_get_sync(drm->dev); 44 45 drm_atomic_helper_commit_modeset_disables(drm, old_state); 46 drm_atomic_helper_commit_planes(drm, old_state, 47 DRM_PLANE_COMMIT_ACTIVE_ONLY); 48 drm_atomic_helper_commit_modeset_enables(drm, old_state); 49 50 drm_atomic_helper_fake_vblank(old_state); 51 drm_atomic_helper_commit_hw_done(old_state); 52 drm_atomic_helper_wait_for_vblanks(drm, old_state); 53 54 pm_runtime_put(drm->dev); 55 56 drm_atomic_helper_cleanup_planes(drm, old_state); 57 } 58
On Friday, September 22, 2023 4:03 AM Lucas Stach <l.stach@pengutronix.de> wrote: > drm_atomic_helper_commit_tail_rpm makes it hard for drivers to follow > the documented encoder/bridge enable flow, as it commits all CRTC enables > before the planes are fully set up, so drivers that can't enable the > display link without valid plane setup either need to do the plane setup > in the CRTC enable or violate the flow by enabling the display link after > the planes have been set up. Neither of those options seem like a good > idea. > > For devices that only do coarse-grained runtime PM for the whole display > controller and not per CRTC, like the i.MX LCDIF, we can handle hardware > wakeup and suspend in the atomic_commit_tail. Add a commit tail which > follows the more conventional atomic commit flow of first diabling any > unused CRTCs, setting up all the active plane state, then enable all > active display pipes and also handles the device runtime PM at the > appropriate times. > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > --- > v2: new patch > --- > drivers/gpu/drm/mxsfb/lcdif_drv.c | 22 +++++++++++++++++++++- > drivers/gpu/drm/mxsfb/lcdif_kms.c | 12 ++++++++++-- > 2 files changed, 31 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c > b/drivers/gpu/drm/mxsfb/lcdif_drv.c > index 18de2f17e249..205f6855fb1b 100644 > --- a/drivers/gpu/drm/mxsfb/lcdif_drv.c > +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c > @@ -36,8 +36,28 @@ static const struct drm_mode_config_funcs > lcdif_mode_config_funcs = { > .atomic_commit = drm_atomic_helper_commit, > }; > > +void lcdif_commit_tail(struct drm_atomic_state *old_state) > +{ > + struct drm_device *drm = old_state->dev; > + > + pm_runtime_get_sync(drm->dev); Here, pixel clock lcdif->clk is enabled via lcdif_rpm_resume(), and then ... > + > + drm_atomic_helper_commit_modeset_disables(drm, old_state); > + drm_atomic_helper_commit_planes(drm, old_state, > + > DRM_PLANE_COMMIT_ACTIVE_ONLY); > + drm_atomic_helper_commit_modeset_enables(drm, old_state); ... here, clk_set_rate() is called for lcdif->clk via lcdif_crtc_atomic_enable(). Set rate with clock enabled? Another concern is lcdif_reset_block() is called via lcdif_crtc_mode_set_nofb() here, while plane is already set up, which means plane settings are potentially reset. With this patch series, display shows constant color by running modetest to change fb pixel format. However, doing page flip with "-v" option seems fine. Also, seems the issue doesn't reproduce without fbdev emulation. Regards, Liu Ying > + > + drm_atomic_helper_fake_vblank(old_state); > + drm_atomic_helper_commit_hw_done(old_state); > + drm_atomic_helper_wait_for_vblanks(drm, old_state); > + > + pm_runtime_put(drm->dev); > + > + drm_atomic_helper_cleanup_planes(drm, old_state); > +} > + > static const struct drm_mode_config_helper_funcs > lcdif_mode_config_helpers = { > - .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm, > + .atomic_commit_tail = lcdif_commit_tail, > }; > > static const struct drm_encoder_funcs lcdif_encoder_funcs = { > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c > b/drivers/gpu/drm/mxsfb/lcdif_kms.c > index e277592e5fa5..ccee5e28f236 100644 > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c > @@ -540,7 +540,11 @@ static void lcdif_crtc_atomic_enable(struct drm_crtc > *crtc, > > clk_set_rate(lcdif->clk, m->crtc_clock * 1000); > > - pm_runtime_get_sync(drm->dev); > + /* > + * Update the RPM usage count, actual resume already happened in > + * lcdif_commit_tail wrapping all the atomic update. > + */ > + pm_runtime_get_noresume(drm->dev); > > lcdif_crtc_mode_set_nofb(new_cstate, new_pstate); > > @@ -576,7 +580,11 @@ static void lcdif_crtc_atomic_disable(struct drm_crtc > *crtc, > } > spin_unlock_irq(&drm->event_lock); > > - pm_runtime_put_sync(drm->dev); > + /* > + * Update the RPM usage count, actual suspend happens in > + * lcdif_commit_tail wrapping all the atomic update. > + */ > + pm_runtime_put(drm->dev); > } > > static void lcdif_crtc_atomic_destroy_state(struct drm_crtc *crtc, > -- > 2.39.2
Am Freitag, dem 22.09.2023 um 09:51 +0000 schrieb Ying Liu: > On Friday, September 22, 2023 4:03 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > drm_atomic_helper_commit_tail_rpm makes it hard for drivers to follow > > the documented encoder/bridge enable flow, as it commits all CRTC enables > > before the planes are fully set up, so drivers that can't enable the > > display link without valid plane setup either need to do the plane setup > > in the CRTC enable or violate the flow by enabling the display link after > > the planes have been set up. Neither of those options seem like a good > > idea. > > > > For devices that only do coarse-grained runtime PM for the whole display > > controller and not per CRTC, like the i.MX LCDIF, we can handle hardware > > wakeup and suspend in the atomic_commit_tail. Add a commit tail which > > follows the more conventional atomic commit flow of first diabling any > > unused CRTCs, setting up all the active plane state, then enable all > > active display pipes and also handles the device runtime PM at the > > appropriate times. > > > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > > --- > > v2: new patch > > --- > > drivers/gpu/drm/mxsfb/lcdif_drv.c | 22 +++++++++++++++++++++- > > drivers/gpu/drm/mxsfb/lcdif_kms.c | 12 ++++++++++-- > > 2 files changed, 31 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c > > b/drivers/gpu/drm/mxsfb/lcdif_drv.c > > index 18de2f17e249..205f6855fb1b 100644 > > --- a/drivers/gpu/drm/mxsfb/lcdif_drv.c > > +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c > > @@ -36,8 +36,28 @@ static const struct drm_mode_config_funcs > > lcdif_mode_config_funcs = { > > .atomic_commit = drm_atomic_helper_commit, > > }; > > > > +void lcdif_commit_tail(struct drm_atomic_state *old_state) > > +{ > > + struct drm_device *drm = old_state->dev; > > + > > + pm_runtime_get_sync(drm->dev); > > Here, pixel clock lcdif->clk is enabled via lcdif_rpm_resume(), and then ... > > > + > > + drm_atomic_helper_commit_modeset_disables(drm, old_state); > > + drm_atomic_helper_commit_planes(drm, old_state, > > + > > DRM_PLANE_COMMIT_ACTIVE_ONLY); > > + drm_atomic_helper_commit_modeset_enables(drm, old_state); > > ... here, clk_set_rate() is called for lcdif->clk via lcdif_crtc_atomic_enable(). > Set rate with clock enabled? > Yea, I don't like the pixel clock enable/disable in the runtime PM handling, but wanted to minimize the changes for now and I don't think there is any issue with changing the rate of a already enabled clock. Might be better to move the pixel clock enable/disable in the atomic_enable/disable to clear any doubt. > Another concern is lcdif_reset_block() is called via lcdif_crtc_mode_set_nofb() > here, while plane is already set up, which means plane settings are potentially > reset. > I thought so as well, but the documentation states that only internal state is reset, not the user visible registers. My testing seemed to indicate that the plane state is unaffected by the reset, but... > With this patch series, display shows constant color by running modetest to > change fb pixel format. However, doing page flip with "-v" option seems fine. > Also, seems the issue doesn't reproduce without fbdev emulation. > ... this seems to contradict this. I'll dig some more into this. I don't even know if this reset is required at all at this point, as it seems this is a leftover from the mxsfb code. I can't find any mandatory reset in the i.MX8MP reference manual. Regards, Lucas > Regards, > Liu Ying > > > + > > + drm_atomic_helper_fake_vblank(old_state); > > + drm_atomic_helper_commit_hw_done(old_state); > > + drm_atomic_helper_wait_for_vblanks(drm, old_state); > > + > > + pm_runtime_put(drm->dev); > > + > > + drm_atomic_helper_cleanup_planes(drm, old_state); > > +} > > + > > static const struct drm_mode_config_helper_funcs > > lcdif_mode_config_helpers = { > > - .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm, > > + .atomic_commit_tail = lcdif_commit_tail, > > }; > > > > static const struct drm_encoder_funcs lcdif_encoder_funcs = { > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c > > b/drivers/gpu/drm/mxsfb/lcdif_kms.c > > index e277592e5fa5..ccee5e28f236 100644 > > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c > > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c > > @@ -540,7 +540,11 @@ static void lcdif_crtc_atomic_enable(struct drm_crtc > > *crtc, > > > > clk_set_rate(lcdif->clk, m->crtc_clock * 1000); > > > > - pm_runtime_get_sync(drm->dev); > > + /* > > + * Update the RPM usage count, actual resume already happened in > > + * lcdif_commit_tail wrapping all the atomic update. > > + */ > > + pm_runtime_get_noresume(drm->dev); > > > > lcdif_crtc_mode_set_nofb(new_cstate, new_pstate); > > > > @@ -576,7 +580,11 @@ static void lcdif_crtc_atomic_disable(struct drm_crtc > > *crtc, > > } > > spin_unlock_irq(&drm->event_lock); > > > > - pm_runtime_put_sync(drm->dev); > > + /* > > + * Update the RPM usage count, actual suspend happens in > > + * lcdif_commit_tail wrapping all the atomic update. > > + */ > > + pm_runtime_put(drm->dev); > > } > > > > static void lcdif_crtc_atomic_destroy_state(struct drm_crtc *crtc, > > -- > > 2.39.2 >
diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c b/drivers/gpu/drm/mxsfb/lcdif_drv.c index 18de2f17e249..205f6855fb1b 100644 --- a/drivers/gpu/drm/mxsfb/lcdif_drv.c +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c @@ -36,8 +36,28 @@ static const struct drm_mode_config_funcs lcdif_mode_config_funcs = { .atomic_commit = drm_atomic_helper_commit, }; +void lcdif_commit_tail(struct drm_atomic_state *old_state) +{ + struct drm_device *drm = old_state->dev; + + pm_runtime_get_sync(drm->dev); + + drm_atomic_helper_commit_modeset_disables(drm, old_state); + drm_atomic_helper_commit_planes(drm, old_state, + DRM_PLANE_COMMIT_ACTIVE_ONLY); + drm_atomic_helper_commit_modeset_enables(drm, old_state); + + drm_atomic_helper_fake_vblank(old_state); + drm_atomic_helper_commit_hw_done(old_state); + drm_atomic_helper_wait_for_vblanks(drm, old_state); + + pm_runtime_put(drm->dev); + + drm_atomic_helper_cleanup_planes(drm, old_state); +} + static const struct drm_mode_config_helper_funcs lcdif_mode_config_helpers = { - .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm, + .atomic_commit_tail = lcdif_commit_tail, }; static const struct drm_encoder_funcs lcdif_encoder_funcs = { diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c index e277592e5fa5..ccee5e28f236 100644 --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c @@ -540,7 +540,11 @@ static void lcdif_crtc_atomic_enable(struct drm_crtc *crtc, clk_set_rate(lcdif->clk, m->crtc_clock * 1000); - pm_runtime_get_sync(drm->dev); + /* + * Update the RPM usage count, actual resume already happened in + * lcdif_commit_tail wrapping all the atomic update. + */ + pm_runtime_get_noresume(drm->dev); lcdif_crtc_mode_set_nofb(new_cstate, new_pstate); @@ -576,7 +580,11 @@ static void lcdif_crtc_atomic_disable(struct drm_crtc *crtc, } spin_unlock_irq(&drm->event_lock); - pm_runtime_put_sync(drm->dev); + /* + * Update the RPM usage count, actual suspend happens in + * lcdif_commit_tail wrapping all the atomic update. + */ + pm_runtime_put(drm->dev); } static void lcdif_crtc_atomic_destroy_state(struct drm_crtc *crtc,
drm_atomic_helper_commit_tail_rpm makes it hard for drivers to follow the documented encoder/bridge enable flow, as it commits all CRTC enables before the planes are fully set up, so drivers that can't enable the display link without valid plane setup either need to do the plane setup in the CRTC enable or violate the flow by enabling the display link after the planes have been set up. Neither of those options seem like a good idea. For devices that only do coarse-grained runtime PM for the whole display controller and not per CRTC, like the i.MX LCDIF, we can handle hardware wakeup and suspend in the atomic_commit_tail. Add a commit tail which follows the more conventional atomic commit flow of first diabling any unused CRTCs, setting up all the active plane state, then enable all active display pipes and also handles the device runtime PM at the appropriate times. Signed-off-by: Lucas Stach <l.stach@pengutronix.de> --- v2: new patch --- drivers/gpu/drm/mxsfb/lcdif_drv.c | 22 +++++++++++++++++++++- drivers/gpu/drm/mxsfb/lcdif_kms.c | 12 ++++++++++-- 2 files changed, 31 insertions(+), 3 deletions(-)