lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 25 Sep 2020 14:29:12 +0200 From: Paul Cercueil <paul@...pouillou.net> To: Sam Ravnborg <sam@...nborg.org>, Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org> Cc: David Airlie <airlied@...ux.ie>, Daniel Vetter <daniel@...ll.ch>, od@...c.me, dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org Subject: Re: [PATCH 2/3] drm/ingenic: Reset pixclock rate when parent clock rate changes Hi Sam, Le jeu. 24 sept. 2020 à 22:22, Sam Ravnborg <sam@...nborg.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@...pouillou.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@...nborg.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
Powered by blists - more mailing lists