| 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
| ||
|
Message-Id: <O0S7HQ.34QHSNHJ7JWJ2@crapouillou.net>
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