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@xxxxxxxxxxxxxxx> > --- > 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@xxxxxxxxxxxx> > + 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel