| 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: <87wn356ni4.fsf@oltmanns.dev>
Date: Sat, 25 Mar 2023 12:40:04 +0100
From: Frank Oltmanns <frank@...manns.dev>
To: Roman Beranek <romanberanek@...oud.com>,
Maxime Ripard <mripard@...nel.org>
Cc: Chen-Yu Tsai <wens@...e.org>, David Airlie <airlied@...il.com>,
Daniel Vetter <daniel@...ll.ch>,
Jernej Skrabec <jernej.skrabec@...il.com>,
Samuel Holland <samuel@...lland.org>,
dri-devel@...ts.freedesktop.org,
linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from
TCON0_DCLK_REG
Hi,
On 2023-03-20 at 17:16:36 +0100, Roman Beranek <romanberanek@...oud.com> wrote:
> In the case of DSI output, the value of SUN4I_TCON0_DCLK_DIV (4) does
> not represent the actual dotclock divider, PLL_MIPI instead runs at
> (bpp / lanes )-multiple [1] of the dotclock. [2] Setting 4 as dotclock
> divder thus leads to reduced frame rate, specifically by 1/3 on 4-lane
> panels, and by 2/3 on 2-lane panels respectively.
>
> As sun4i_dotclock driver stores its calculated divider directly in
> the register, conditional handling of the DSI output scenario is needed.
> Instead of reading the divider from SUN4I_TCON0_DCLK_REG, retrieve
> the value from tcon->dclk_min_div.
>
> [1] bits per pixel / number of DSI lanes
> [2] <https://github.com/BPI-SINOVOIP/BPI-M64-bsp-4.4/blob/66bef0f2f30b367eb93b1cbad21ce85e0361f7ae/linux-sunxi/drivers/video/fbdev/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L322>
>
> Signed-off-by: Roman Beranek <romanberanek@...oud.com>
> —
> drivers/gpu/drm/sun4i/sun4i_dotclock.c | 6 +++++-
> drivers/gpu/drm/sun4i/sun4i_tcon.c | 5 +++–
> drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 +
> 3 files changed, 9 insertions(+), 3 deletions(-)
>
> diff –git a/drivers/gpu/drm/sun4i/sun4i_dotclock.c b/drivers/gpu/drm/sun4i/sun4i_dotclock.c
> index 417ade3d2565..26fa99aff590 100644
> — a/drivers/gpu/drm/sun4i/sun4i_dotclock.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_dotclock.c
> @@ -11,6 +11,7 @@
>
> #include “sun4i_tcon.h”
> #include “sun4i_dotclock.h”
> +#include “sun6i_mipi_dsi.h”
>
> struct sun4i_dclk {
> struct clk_hw hw;
> @@ -56,6 +57,9 @@ static unsigned long sun4i_dclk_recalc_rate(struct clk_hw *hw,
> struct sun4i_dclk *dclk = hw_to_dclk(hw);
> u32 val;
>
> + if (dclk->tcon->is_dsi)
> + return parent_rate / dclk->tcon->dclk_min_div;
> +
> regmap_read(dclk->regmap, SUN4I_TCON0_DCLK_REG, &val);
>
> val >>= SUN4I_TCON0_DCLK_DIV_SHIFT;
> @@ -116,7 +120,7 @@ static int sun4i_dclk_set_rate(struct clk_hw *hw, unsigned long rate,
> unsigned long parent_rate)
> {
> struct sun4i_dclk *dclk = hw_to_dclk(hw);
> - u8 div = parent_rate / rate;
> + u8 div = dclk->tcon->is_dsi ? SUN6I_DSI_TCON_DIV : parent_rate / rate;
>
> return regmap_update_bits(dclk->regmap, SUN4I_TCON0_DCLK_REG,
> GENMASK(6, 0), div);
> diff –git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index 523a6d787921..7f5d3c135058 100644
> — a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -367,8 +367,9 @@ static void sun4i_tcon0_mode_set_cpu(struct sun4i_tcon *tcon,
> u32 block_space, start_delay;
> u32 tcon_div;
>
> - tcon->dclk_min_div = SUN6I_DSI_TCON_DIV;
> - tcon->dclk_max_div = SUN6I_DSI_TCON_DIV;
> + tcon->is_dsi = true;
> + tcon->dclk_min_div = bpp / lanes;
> + tcon->dclk_max_div = bpp / lanes;
Claiming to set the divider to a different value (bpp / lanes) than what we’re actually using in the end (SUN6I_DSIO_TCON_DIV) is somehow bugging me. I feel like the proposal that I submitted is more direct:
<https://lore.kernel.org/all/20230319160704.9858-2-frank@oltmanns.dev/>
Actually, I had the following third patch prepared that adjusted the dotclock rate so that the required PLL rate is set. But again, this seems very indirect, so that’s why I refrained from submitting it and I submitted the linked patch above instead.
Anyway, here is the third proposal:
— a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -819,6 +819,34 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
regulator_disable(dsi->regulator);
}
+static bool sun6i_dsi_encoder_mode_fixup(
⁃ struct drm_encoder *encoder,
⁃ const struct drm_display_mode *mode,
⁃ struct drm_display_mode *adjusted_mode)
+{
⁃ if (encoder->encoder_type == DRM_MODE_ENCODER_DSI) {
⁃ /*
⁃ * For DSI the PLL rate has to respect the bits per pixel and
⁃ * number of lanes.
⁃ *
⁃ * According to the BSP code:
⁃ * PLL rate = DOTCLOCK * bpp / lanes
⁃ *
⁃ * Therefore, the clock has to be adjusted in order to set the
⁃ * correct PLL rate when actually setting the clock.
⁃ */
⁃ struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder);
⁃ struct mipi_dsi_device *device = dsi->device;
⁃ u8 bpp = mipi_dsi_pixel_format_to_bpp(device->format);
⁃ u8 lanes = device->lanes;
⁃
⁃ adjusted_mode->crtc_clock = mode->crtc_clock
⁃ * bpp / (lanes * SUN6I_DSI_TCON_DIV);
⁃ }
⁃
⁃ return true;
+}
⁃ static int sun6i_dsi_get_modes(struct drm_connector *connector)
{
struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector);
@@ -851,6 +879,7 @@ static const struct drm_connector_funcs sun6i_dsi_connector_funcs = {
static const struct drm_encoder_helper_funcs sun6i_dsi_enc_helper_funcs = {
.disable = sun6i_dsi_encoder_disable,
.enable = sun6i_dsi_encoder_enable,
⁃ .mode_fixup = sun6i_dsi_encoder_mode_fixup,
};
static u32 sun6i_dsi_dcs_build_pkt_hdr(struct sun6i_dsi *dsi,
Maxime, Roman, CC, what do you think? Can we achieve consensus? If I’m not mistaken, all of the three proposal are a step in the right direction, because they correct faulty behavior. Don’t you think?
Thanks,
Frank
>
> sun4i_tcon0_mode_set_common(tcon, mode);
>
> diff –git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h
> index fa23aa23fe4a..d8150ba2f319 100644
> — a/drivers/gpu/drm/sun4i/sun4i_tcon.h
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h
> @@ -271,6 +271,7 @@ struct sun4i_tcon {
> struct clk *dclk;
> u8 dclk_max_div;
> u8 dclk_min_div;
> + bool is_dsi;
>
> /* Reset control */
> struct reset_control *lcd_rst;
Powered by blists - more mailing lists