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

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@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

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

[Stephen Boyd]

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.