Re: [PATCH 2/3] drm/ingenic: Reset pixclock rate when parent clock rate changes

[Paul Cercueil]

Hi Sam,

Le jeu. 24 sept. 2020 à 22:22, Sam Ravnborg <sam@xxxxxxxxxxxx> 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@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.

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