Re: [PATCH 10/11] drm/bridge: tc358768: Attempt to fix DSI horizontal timings
On 04/08/2023 13:44, Tomi Valkeinen wrote:
> The DSI horizontal timing calculations done by the driver seem to often
> lead to underflows or overflows, depending on the videomode.
>
> There are two main things the current driver doesn't seem to get right:
> DSI HSW and HFP, and VSDly. However, even following Toshiba's
> documentation it seems we don't always get a working display.
>
> This patch attempts to fix the horizontal timings for DSI event mode, and
> on a system with a DSI->HDMI encoder, a lot of standard HDMI modes now
> seem to work. The work relies on Toshiba's documentation, but also quite
> a bit on empirical testing.
>
> This also adds timing related debug prints to make it easier to improve
> on this later.
>
> The DSI pulse mode has only been tested with a fixed-resolution panel,
> which limits the testing of different modes on DSI pulse mode. However,
> as the VSDly calculation also affects pulse mode, so this might cause a
> regression.
If my memory serves me right we only had this used in a sinlge, static
configuration and again, it might be mentioned in a comment somewhere?
Nice work!
Reviewed-by: Peter Ujfalusi
>
> Signed-off-by: Tomi Valkeinen
> ---
> drivers/gpu/drm/bridge/tc358768.c | 211
> +-
> 1 file changed, 183 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358768.c
> b/drivers/gpu/drm/bridge/tc358768.c
> index dc2241c18dde..ea19de5509ed 100644
> --- a/drivers/gpu/drm/bridge/tc358768.c
> +++ b/drivers/gpu/drm/bridge/tc358768.c
> @@ -9,6 +9,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -157,6 +158,7 @@ struct tc358768_priv {
> u32 frs;/* PLL Freqency range for HSCK (post divider) */
>
> u32 dsiclk; /* pll_clk / 2 */
> + u32 pclk; /* incoming pclk rate */
> };
>
> static inline struct tc358768_priv *dsi_host_to_tc358768(struct mipi_dsi_host
> @@ -380,6 +382,7 @@ static int tc358768_calc_pll(struct tc358768_priv *priv,
> priv->prd = best_prd;
> priv->frs = frs;
> priv->dsiclk = best_pll / 2;
> + priv->pclk = mode->clock * 1000;
>
> return 0;
> }
> @@ -638,6 +641,28 @@ static u32 tc358768_ps_to_ns(u32 ps)
> return ps / 1000;
> }
>
> +static u32 tc358768_dpi_to_ns(u32 val, u32 pclk)
> +{
> + return (u32)div_u64((u64)val * NANO, pclk);
> +}
> +
> +/* Convert value in DPI pixel clock units to DSI byte count */
> +static u32 tc358768_dpi_to_dsi_bytes(struct tc358768_priv *priv, u32 val)
> +{
> + u64 m = (u64)val * priv->dsiclk / 4 * priv->dsi_lanes;
> + u64 n = priv->pclk;
> +
> + return (u32)div_u64(m + n - 1, n);
> +}
> +
> +static u32 tc358768_dsi_bytes_to_ns(struct tc358768_priv *priv, u32 val)
> +{
> + u64 m = (u64)val * NANO;
> + u64 n = priv->dsiclk / 4 * priv->dsi_lanes;
> +
> + return (u32)div_u64(m, n);
> +}
> +
> static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
> {
> struct tc358768_priv *priv = bridge_to_tc358768(bridge);
> @@ -647,11 +672,19 @@ static void tc358768_bridge_pre_enable(struct
> drm_bridge *bridge)
> s32 raw_val;
> const struct drm_display_mode *mode;
> u32 hsbyteclk_ps, dsiclk_ps, ui_ps;
> - u32 dsiclk, hsbyteclk, video_start;
> - const u32 internal_delay = 40;
> + u32 dsiclk, hsbyteclk;
> int ret, i;
> struct videomode vm;
> struct device *dev = priv->dev;
> + /* In pixelclock units */
> + u32 dpi_htot, dpi_data_start;
> + /* In byte units */
> + u32 dsi_dpi_htot, dsi_dpi_data_start;
> + u32 dsi_hsw, dsi_hbp, dsi_hact, dsi_hfp;
> + const u32 dsi_hss = 4; /* HSS is a short packet (4 bytes) */
> + /* In hsbyteclk units */
> + u32 dsi_vsdly;
> + const u32 internal_dly = 40;
>
> if (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) {
> dev_warn_once(dev, "Non-continuous mode unimplemented, falling
> back to continuous\n");
> @@ -686,27 +719,23 @@ static void tc358768_bridge_pre_enable(struct
> drm_bridge *bridge)
> case MIPI_DSI_FMT_RGB888:
> val |= (0x3 << 4);
> hact = vm.hactive * 3;
> - video_start = (vm.hsync_len + vm.hback_porch) * 3;
> data_type = MIPI_DSI_PACKED_PIXEL_STREAM_24;
> break;
> case MIPI_DSI_FMT_RGB666:
> val |= (0x4 << 4);
> hact = vm.hactive * 3;
> - video_start = (vm.hsync_len + vm.hback_porch) * 3;
> data_type = MIPI_DSI_PACKED_PIXEL_STREAM_18;
> break;
>
> case MIPI_DSI_FMT_RGB666_PACKED:
> val |= (0x4 << 4) | BIT(3);
> hact = vm.hactive * 18 / 8;
> - video_start = (vm.hsync_len + vm.hback_porch) * 18 / 8;
> data_type = MIPI_DSI_PIXEL_STREAM_3BYTE_18;
> break;
>
> case MIPI_DSI_FMT_RGB565:
> val |= (0x5 << 4);
> hact
[PATCH 10/11] drm/bridge: tc358768: Attempt to fix DSI horizontal timings
The DSI horizontal timing calculations done by the driver seem to often
lead to underflows or overflows, depending on the videomode.
There are two main things the current driver doesn't seem to get right:
DSI HSW and HFP, and VSDly. However, even following Toshiba's
documentation it seems we don't always get a working display.
This patch attempts to fix the horizontal timings for DSI event mode, and
on a system with a DSI->HDMI encoder, a lot of standard HDMI modes now
seem to work. The work relies on Toshiba's documentation, but also quite
a bit on empirical testing.
This also adds timing related debug prints to make it easier to improve
on this later.
The DSI pulse mode has only been tested with a fixed-resolution panel,
which limits the testing of different modes on DSI pulse mode. However,
as the VSDly calculation also affects pulse mode, so this might cause a
regression.
Signed-off-by: Tomi Valkeinen
---
drivers/gpu/drm/bridge/tc358768.c | 211 +-
1 file changed, 183 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358768.c
b/drivers/gpu/drm/bridge/tc358768.c
index dc2241c18dde..ea19de5509ed 100644
--- a/drivers/gpu/drm/bridge/tc358768.c
+++ b/drivers/gpu/drm/bridge/tc358768.c
@@ -9,6 +9,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -157,6 +158,7 @@ struct tc358768_priv {
u32 frs;/* PLL Freqency range for HSCK (post divider) */
u32 dsiclk; /* pll_clk / 2 */
+ u32 pclk; /* incoming pclk rate */
};
static inline struct tc358768_priv *dsi_host_to_tc358768(struct mipi_dsi_host
@@ -380,6 +382,7 @@ static int tc358768_calc_pll(struct tc358768_priv *priv,
priv->prd = best_prd;
priv->frs = frs;
priv->dsiclk = best_pll / 2;
+ priv->pclk = mode->clock * 1000;
return 0;
}
@@ -638,6 +641,28 @@ static u32 tc358768_ps_to_ns(u32 ps)
return ps / 1000;
}
+static u32 tc358768_dpi_to_ns(u32 val, u32 pclk)
+{
+ return (u32)div_u64((u64)val * NANO, pclk);
+}
+
+/* Convert value in DPI pixel clock units to DSI byte count */
+static u32 tc358768_dpi_to_dsi_bytes(struct tc358768_priv *priv, u32 val)
+{
+ u64 m = (u64)val * priv->dsiclk / 4 * priv->dsi_lanes;
+ u64 n = priv->pclk;
+
+ return (u32)div_u64(m + n - 1, n);
+}
+
+static u32 tc358768_dsi_bytes_to_ns(struct tc358768_priv *priv, u32 val)
+{
+ u64 m = (u64)val * NANO;
+ u64 n = priv->dsiclk / 4 * priv->dsi_lanes;
+
+ return (u32)div_u64(m, n);
+}
+
static void tc358768_bridge_pre_enable(struct drm_bridge *bridge)
{
struct tc358768_priv *priv = bridge_to_tc358768(bridge);
@@ -647,11 +672,19 @@ static void tc358768_bridge_pre_enable(struct drm_bridge
*bridge)
s32 raw_val;
const struct drm_display_mode *mode;
u32 hsbyteclk_ps, dsiclk_ps, ui_ps;
- u32 dsiclk, hsbyteclk, video_start;
- const u32 internal_delay = 40;
+ u32 dsiclk, hsbyteclk;
int ret, i;
struct videomode vm;
struct device *dev = priv->dev;
+ /* In pixelclock units */
+ u32 dpi_htot, dpi_data_start;
+ /* In byte units */
+ u32 dsi_dpi_htot, dsi_dpi_data_start;
+ u32 dsi_hsw, dsi_hbp, dsi_hact, dsi_hfp;
+ const u32 dsi_hss = 4; /* HSS is a short packet (4 bytes) */
+ /* In hsbyteclk units */
+ u32 dsi_vsdly;
+ const u32 internal_dly = 40;
if (mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) {
dev_warn_once(dev, "Non-continuous mode unimplemented, falling
back to continuous\n");
@@ -686,27 +719,23 @@ static void tc358768_bridge_pre_enable(struct drm_bridge
*bridge)
case MIPI_DSI_FMT_RGB888:
val |= (0x3 << 4);
hact = vm.hactive * 3;
- video_start = (vm.hsync_len + vm.hback_porch) * 3;
data_type = MIPI_DSI_PACKED_PIXEL_STREAM_24;
break;
case MIPI_DSI_FMT_RGB666:
val |= (0x4 << 4);
hact = vm.hactive * 3;
- video_start = (vm.hsync_len + vm.hback_porch) * 3;
data_type = MIPI_DSI_PACKED_PIXEL_STREAM_18;
break;
case MIPI_DSI_FMT_RGB666_PACKED:
val |= (0x4 << 4) | BIT(3);
hact = vm.hactive * 18 / 8;
- video_start = (vm.hsync_len + vm.hback_porch) * 18 / 8;
data_type = MIPI_DSI_PIXEL_STREAM_3BYTE_18;
break;
case MIPI_DSI_FMT_RGB565:
val |= (0x5 << 4);
hact = vm.hactive * 2;
- video_start = (vm.hsync_len + vm.hback_porch) * 2;
data_type = MIPI_DSI_PACKED_PIXEL_STREAM_16;
break;
default:
@@ -716,9 +745,150 @@ static void tc358768_bridge_pre_enable(struct drm_bridge
*bridge)
return;
}
+ /*
+* There are three important things to make TC358768
