| Message ID | 20200915123818.13272-3-paul@crapouillou.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Small improvements to ingenic-drm | expand |
Hi Paul. On Tue, Sep 15, 2020 at 02:38:17PM +0200, Paul Cercueil wrote: > Old Ingenic SoCs can overclock very well, up to +50% of their nominal > clock rate, whithout requiring overvolting or anything like that, just > by changing the rate of the main PLL. Unfortunately, all clocks on the > system are derived from that PLL, and when the PLL rate is updated, so > is our pixel clock. > > To counter that issue, we make sure that the panel is in VBLANK before > the rate change happens, and we will then re-set the pixel clock rate > afterwards, once the PLL has been changed, to be as close as possible to > the pixel rate requested by the encoder. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 49 ++++++++++++++++++++++- > 1 file changed, 48 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > index fb62869befdc..aa32660033d2 100644 > --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > @@ -12,6 +12,7 @@ > #include <linux/dma-noncoherent.h> > #include <linux/io.h> > #include <linux/module.h> > +#include <linux/mutex.h> > #include <linux/of_device.h> > #include <linux/platform_device.h> > #include <linux/regmap.h> > @@ -73,6 +74,9 @@ struct ingenic_drm { > > bool panel_is_sharp; > bool no_vblank; > + bool update_clk_rate; > + struct mutex clk_mutex; Please add comment about what the mutex protects. Especially since the mutex is locked and unlocked based on a notification. With the comment added: Acked-by: Sam Ravnborg <sam@ravnborg.org> > + struct notifier_block clock_nb; > }; > > static bool ingenic_drm_cached_gem_buf; > @@ -115,6 +119,29 @@ static inline struct ingenic_drm *drm_crtc_get_priv(struct drm_crtc *crtc) > return container_of(crtc, struct ingenic_drm, crtc); > } > > +static inline struct ingenic_drm *drm_nb_get_priv(struct notifier_block *nb) > +{ > + return container_of(nb, struct ingenic_drm, clock_nb); > +} > + > +static int ingenic_drm_update_pixclk(struct notifier_block *nb, > + unsigned long action, > + void *data) > +{ > + struct ingenic_drm *priv = drm_nb_get_priv(nb); > + > + switch (action) { > + case PRE_RATE_CHANGE: > + mutex_lock(&priv->clk_mutex); > + priv->update_clk_rate = true; > + drm_crtc_wait_one_vblank(&priv->crtc); > + return NOTIFY_OK; > + default: > + mutex_unlock(&priv->clk_mutex); Any risk the POST_RATE_CHANGE or ABORT_RATE_CHANGE may go missing so we fail to unlock the mutex? I think not but wanted to make sure you had thought about it. > + return NOTIFY_OK; > + } > +} > + > static void ingenic_drm_crtc_atomic_enable(struct drm_crtc *crtc, > struct drm_crtc_state *state) > { > @@ -280,8 +307,14 @@ static void ingenic_drm_crtc_atomic_flush(struct drm_crtc *crtc, > > if (drm_atomic_crtc_needs_modeset(state)) { > ingenic_drm_crtc_update_timings(priv, &state->mode); > + priv->update_clk_rate = true; > + } > > + if (priv->update_clk_rate) { > + mutex_lock(&priv->clk_mutex); > clk_set_rate(priv->pix_clk, state->adjusted_mode.clock * 1000); > + priv->update_clk_rate = false; > + mutex_unlock(&priv->clk_mutex); > } > > if (event) { > @@ -1046,16 +1079,28 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) > if (soc_info->has_osd) > regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN); > > + mutex_init(&priv->clk_mutex); > + priv->clock_nb.notifier_call = ingenic_drm_update_pixclk; > + > + parent_clk = clk_get_parent(priv->pix_clk); > + ret = clk_notifier_register(parent_clk, &priv->clock_nb); > + if (ret) { > + dev_err(dev, "Unable to register clock notifier\n"); > + goto err_devclk_disable; > + } > + > ret = drm_dev_register(drm, 0); > if (ret) { > dev_err(dev, "Failed to register DRM driver\n"); > - goto err_devclk_disable; > + goto err_clk_notifier_unregister; > } > > drm_fbdev_generic_setup(drm, 32); > > return 0; > > +err_clk_notifier_unregister: > + clk_notifier_unregister(parent_clk, &priv->clock_nb); > err_devclk_disable: > if (priv->lcd_clk) > clk_disable_unprepare(priv->lcd_clk); > @@ -1077,7 +1122,9 @@ static int compare_of(struct device *dev, void *data) > static void ingenic_drm_unbind(struct device *dev) > { > struct ingenic_drm *priv = dev_get_drvdata(dev); > + struct clk *parent_clk = clk_get_parent(priv->pix_clk); > > + clk_notifier_unregister(parent_clk, &priv->clock_nb); > if (priv->lcd_clk) > clk_disable_unprepare(priv->lcd_clk); > clk_disable_unprepare(priv->pix_clk); > -- > 2.28.0
Hi Sam, Le jeu. 24 sept. 2020 à 22:22, Sam Ravnborg <sam@ravnborg.org> a écrit : > Hi Paul. > > On Tue, Sep 15, 2020 at 02:38:17PM +0200, Paul Cercueil wrote: >> Old Ingenic SoCs can overclock very well, up to +50% of their >> nominal >> clock rate, whithout requiring overvolting or anything like that, >> just >> by changing the rate of the main PLL. Unfortunately, all clocks on >> the >> system are derived from that PLL, and when the PLL rate is updated, >> so >> is our pixel clock. >> >> To counter that issue, we make sure that the panel is in VBLANK >> before >> the rate change happens, and we will then re-set the pixel clock >> rate >> afterwards, once the PLL has been changed, to be as close as >> possible to >> the pixel rate requested by the encoder. >> >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> >> --- >> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 49 >> ++++++++++++++++++++++- >> 1 file changed, 48 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c >> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c >> index fb62869befdc..aa32660033d2 100644 >> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c >> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c >> @@ -12,6 +12,7 @@ >> #include <linux/dma-noncoherent.h> >> #include <linux/io.h> >> #include <linux/module.h> >> +#include <linux/mutex.h> >> #include <linux/of_device.h> >> #include <linux/platform_device.h> >> #include <linux/regmap.h> >> @@ -73,6 +74,9 @@ struct ingenic_drm { >> >> bool panel_is_sharp; >> bool no_vblank; >> + bool update_clk_rate; >> + struct mutex clk_mutex; > Please add comment about what the mutex protects. > Especially since the mutex is locked and unlocked based on a > notification. > > With the comment added: > Acked-by: Sam Ravnborg <sam@ravnborg.org> > >> + struct notifier_block clock_nb; >> }; >> >> static bool ingenic_drm_cached_gem_buf; >> @@ -115,6 +119,29 @@ static inline struct ingenic_drm >> *drm_crtc_get_priv(struct drm_crtc *crtc) >> return container_of(crtc, struct ingenic_drm, crtc); >> } >> >> +static inline struct ingenic_drm *drm_nb_get_priv(struct >> notifier_block *nb) >> +{ >> + return container_of(nb, struct ingenic_drm, clock_nb); >> +} >> + >> +static int ingenic_drm_update_pixclk(struct notifier_block *nb, >> + unsigned long action, >> + void *data) >> +{ >> + struct ingenic_drm *priv = drm_nb_get_priv(nb); >> + >> + switch (action) { >> + case PRE_RATE_CHANGE: >> + mutex_lock(&priv->clk_mutex); >> + priv->update_clk_rate = true; >> + drm_crtc_wait_one_vblank(&priv->crtc); >> + return NOTIFY_OK; >> + default: >> + mutex_unlock(&priv->clk_mutex); > Any risk the POST_RATE_CHANGE or ABORT_RATE_CHANGE may go missing so > we > fail to unlock the mutex? > I think not but wanted to make sure you had thought about it. My assumption was that you always get POST_RATE_CHANGE or ABORT_RATE_CHANGE. But I am not 100% sure about that. Michael, Stephen: is it safe to assume that I will always get notified with POST_RATE_CHANGE or ABORT_RATE_CHANGE, after I got notified with PRE_RATE_CHANGE? Thanks, -Paul >> + return NOTIFY_OK; >> + } >> +} >> + >> static void ingenic_drm_crtc_atomic_enable(struct drm_crtc *crtc, >> struct drm_crtc_state *state) >> { >> @@ -280,8 +307,14 @@ static void >> ingenic_drm_crtc_atomic_flush(struct drm_crtc *crtc, >> >> if (drm_atomic_crtc_needs_modeset(state)) { >> ingenic_drm_crtc_update_timings(priv, &state->mode); >> + priv->update_clk_rate = true; >> + } >> >> + if (priv->update_clk_rate) { >> + mutex_lock(&priv->clk_mutex); >> clk_set_rate(priv->pix_clk, state->adjusted_mode.clock * 1000); >> + priv->update_clk_rate = false; >> + mutex_unlock(&priv->clk_mutex); >> } >> >> if (event) { >> @@ -1046,16 +1079,28 @@ static int ingenic_drm_bind(struct device >> *dev, bool has_components) >> if (soc_info->has_osd) >> regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN); >> >> + mutex_init(&priv->clk_mutex); >> + priv->clock_nb.notifier_call = ingenic_drm_update_pixclk; >> + >> + parent_clk = clk_get_parent(priv->pix_clk); >> + ret = clk_notifier_register(parent_clk, &priv->clock_nb); >> + if (ret) { >> + dev_err(dev, "Unable to register clock notifier\n"); >> + goto err_devclk_disable; >> + } >> + >> ret = drm_dev_register(drm, 0); >> if (ret) { >> dev_err(dev, "Failed to register DRM driver\n"); >> - goto err_devclk_disable; >> + goto err_clk_notifier_unregister; >> } >> >> drm_fbdev_generic_setup(drm, 32); >> >> return 0; >> >> +err_clk_notifier_unregister: >> + clk_notifier_unregister(parent_clk, &priv->clock_nb); >> err_devclk_disable: >> if (priv->lcd_clk) >> clk_disable_unprepare(priv->lcd_clk); >> @@ -1077,7 +1122,9 @@ static int compare_of(struct device *dev, >> void *data) >> static void ingenic_drm_unbind(struct device *dev) >> { >> struct ingenic_drm *priv = dev_get_drvdata(dev); >> + struct clk *parent_clk = clk_get_parent(priv->pix_clk); >> >> + clk_notifier_unregister(parent_clk, &priv->clock_nb); >> if (priv->lcd_clk) >> clk_disable_unprepare(priv->lcd_clk); >> clk_disable_unprepare(priv->pix_clk); >> -- >> 2.28.0
Quoting Paul Cercueil (2020-09-25 05:29:12) > >> +static int ingenic_drm_update_pixclk(struct notifier_block *nb, > >> + unsigned long action, > >> + void *data) > >> +{ > >> + struct ingenic_drm *priv = drm_nb_get_priv(nb); > >> + > >> + switch (action) { > >> + case PRE_RATE_CHANGE: > >> + mutex_lock(&priv->clk_mutex); > >> + priv->update_clk_rate = true; > >> + drm_crtc_wait_one_vblank(&priv->crtc); > >> + return NOTIFY_OK; > >> + default: > >> + mutex_unlock(&priv->clk_mutex); > > Any risk the POST_RATE_CHANGE or ABORT_RATE_CHANGE may go missing so > > we > > fail to unlock the mutex? > > I think not but wanted to make sure you had thought about it. > > My assumption was that you always get POST_RATE_CHANGE or > ABORT_RATE_CHANGE. But I am not 100% sure about that. > > Michael, Stephen: is it safe to assume that I will always get notified > with POST_RATE_CHANGE or ABORT_RATE_CHANGE, after I got notified with > PRE_RATE_CHANGE? > I think one or the other will happen. Of course, the notifiers are sort of shunned so if you can avoid using notifiers entirely it would be better.
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index fb62869befdc..aa32660033d2 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -12,6 +12,7 @@ #include <linux/dma-noncoherent.h> #include <linux/io.h> #include <linux/module.h> +#include <linux/mutex.h> #include <linux/of_device.h> #include <linux/platform_device.h> #include <linux/regmap.h> @@ -73,6 +74,9 @@ struct ingenic_drm { bool panel_is_sharp; bool no_vblank; + bool update_clk_rate; + struct mutex clk_mutex; + struct notifier_block clock_nb; }; static bool ingenic_drm_cached_gem_buf; @@ -115,6 +119,29 @@ static inline struct ingenic_drm *drm_crtc_get_priv(struct drm_crtc *crtc) return container_of(crtc, struct ingenic_drm, crtc); } +static inline struct ingenic_drm *drm_nb_get_priv(struct notifier_block *nb) +{ + return container_of(nb, struct ingenic_drm, clock_nb); +} + +static int ingenic_drm_update_pixclk(struct notifier_block *nb, + unsigned long action, + void *data) +{ + struct ingenic_drm *priv = drm_nb_get_priv(nb); + + switch (action) { + case PRE_RATE_CHANGE: + mutex_lock(&priv->clk_mutex); + priv->update_clk_rate = true; + drm_crtc_wait_one_vblank(&priv->crtc); + return NOTIFY_OK; + default: + mutex_unlock(&priv->clk_mutex); + return NOTIFY_OK; + } +} + static void ingenic_drm_crtc_atomic_enable(struct drm_crtc *crtc, struct drm_crtc_state *state) { @@ -280,8 +307,14 @@ static void ingenic_drm_crtc_atomic_flush(struct drm_crtc *crtc, if (drm_atomic_crtc_needs_modeset(state)) { ingenic_drm_crtc_update_timings(priv, &state->mode); + priv->update_clk_rate = true; + } + if (priv->update_clk_rate) { + mutex_lock(&priv->clk_mutex); clk_set_rate(priv->pix_clk, state->adjusted_mode.clock * 1000); + priv->update_clk_rate = false; + mutex_unlock(&priv->clk_mutex); } if (event) { @@ -1046,16 +1079,28 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) if (soc_info->has_osd) regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN); + mutex_init(&priv->clk_mutex); + priv->clock_nb.notifier_call = ingenic_drm_update_pixclk; + + parent_clk = clk_get_parent(priv->pix_clk); + ret = clk_notifier_register(parent_clk, &priv->clock_nb); + if (ret) { + dev_err(dev, "Unable to register clock notifier\n"); + goto err_devclk_disable; + } + ret = drm_dev_register(drm, 0); if (ret) { dev_err(dev, "Failed to register DRM driver\n"); - goto err_devclk_disable; + goto err_clk_notifier_unregister; } drm_fbdev_generic_setup(drm, 32); return 0; +err_clk_notifier_unregister: + clk_notifier_unregister(parent_clk, &priv->clock_nb); err_devclk_disable: if (priv->lcd_clk) clk_disable_unprepare(priv->lcd_clk); @@ -1077,7 +1122,9 @@ static int compare_of(struct device *dev, void *data) static void ingenic_drm_unbind(struct device *dev) { struct ingenic_drm *priv = dev_get_drvdata(dev); + struct clk *parent_clk = clk_get_parent(priv->pix_clk); + clk_notifier_unregister(parent_clk, &priv->clock_nb); if (priv->lcd_clk) clk_disable_unprepare(priv->lcd_clk); clk_disable_unprepare(priv->pix_clk);
Old Ingenic SoCs can overclock very well, up to +50% of their nominal clock rate, whithout requiring overvolting or anything like that, just by changing the rate of the main PLL. Unfortunately, all clocks on the system are derived from that PLL, and when the PLL rate is updated, so is our pixel clock. To counter that issue, we make sure that the panel is in VBLANK before the rate change happens, and we will then re-set the pixel clock rate afterwards, once the PLL has been changed, to be as close as possible to the pixel rate requested by the encoder. Signed-off-by: Paul Cercueil <paul@crapouillou.net> --- drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 49 ++++++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-)