| 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:
<BY5PR04MB67398ABD41B3BACED4423EEBC7AD2@BY5PR04MB6739.namprd04.prod.outlook.com>
Date: Mon, 31 Mar 2025 08:29:35 +0000
From: Xin Ji <xji@...logixsemi.com>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
CC: Andrzej Hajda <andrzej.hajda@...el.com>, Neil Armstrong
<neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>, Laurent Pinchart
<Laurent.pinchart@...asonboard.com>, Jonas Karlman <jonas@...boo.se>, Jernej
Skrabec <jernej.skrabec@...il.com>, Maarten Lankhorst
<maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>,
Simona Vetter <simona@...ll.ch>, Bernie Liang <bliang@...logixsemi.com>,
Qilin Wen <qwen@...logixsemi.com>, "treapking@...omium.org"
<treapking@...omium.org>, "dri-devel@...ts.freedesktop.org"
<dri-devel@...ts.freedesktop.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2] drm/bridge:anx7625: Enable DSC feature
> > > > > 4K30(3840x2160 30Hz) timing pixel clock around 297M, for 24bits
> > > > > RGB pixel data format, total transport bandwidth need 297M*24(at
> > > > > least
> > > > > 7.2Gbps) more than anx7625 mipi rx lane bandwidth(maximum 6Gbps,
> > > > > 4lanes, each lane 1.5Gbps). Without DSC function, anx7625 cannot
> > > > > receive 4K30 video timing.
> > > > >
> > > > > When display pixel clock exceed 250M, driver will enable DSC
> > > > > feature, and the compression ratio is 3:1, eg: 4K30's pixel
> > > > > clock around 297M, bandwidth 7.2G will be compressed to 7.2G/3 =
> > > > > 2.4G. Then anx7625 can receive 4K30 video timing and do
> > > > > decompress, then package video data and send to sink device through DP
> link.
> > > > >
> > > > > Anx7625 is bridge IC, sink monitor only receive normal DP signal
> > > > > from anx7625, sink device didn't know DSC information between
> > > > > SOC and
> > > > > anx7625
> > > > >
> > > > > v2:
> > > > > 1. Add more commit message
> > > > > 2. Remove unused code
> > > > > 3. Add more comment
> > > >
> > > > This part is useless, it adds no information. It is as good as 'changed it'.
> > > OK, I'll remove it
> > > >
> > > > > 4. Remove dsc_en flag
> > > > >
> > > > > Signed-off-by: Xin Ji <xji@...logixsemi.com>
> > > > > ---
> > > > > drivers/gpu/drm/bridge/analogix/anx7625.c | 299
> > > > > ++++++++++++++++++----
> > > > > ++++++++++++++++++drivers/gpu/drm/bridge/analogix/anx7625.h |
> > > > > 31 +++
> > > > > 2 files changed, 274 insertions(+), 56 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > > > b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > > > index 4be34d5c7a3b..bae76d9a665f 100644
> > > > > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > > > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > > > @@ -9,6 +9,7 @@
> > > > > #include <linux/interrupt.h>
> > > > > #include <linux/iopoll.h>
> > > > > #include <linux/kernel.h>
> > > > > +#include <linux/math64.h>
> > > > > #include <linux/module.h>
> > > > > #include <linux/mutex.h>
> > > > > #include <linux/pm_runtime.h>
> > > > > @@ -22,6 +23,7 @@
> > > > >
> > > > > #include <drm/display/drm_dp_aux_bus.h> #include
> > > > > <drm/display/drm_dp_helper.h>
> > > > > +#include <drm/display/drm_dsc_helper.h>
> > > > > #include <drm/display/drm_hdcp_helper.h> #include
> > > > > <drm/drm_atomic_helper.h> #include <drm/drm_bridge.h> @@
> > > > > -476,11
> > > > > +478,159 @@ static int anx7625_set_k_value(struct anx7625_data
> > > > > +*ctx)
> > > > > MIPI_DIGITAL_ADJ_1, 0x3D); }
> > > > >
> > > > > +static bool anx7625_dsc_check(struct anx7625_data *ctx) {
> > > > > + if (ctx->dt.pixelclock.min > DSC_PIXEL_CLOCK)
> > > > > + return true;
> > > >
> > > > So, now DSC is enabled unconditionally just because the clock is
> > > > too high. Let's see...
> > > Yes
> > > >
> > > > > +
> > > > > + return false;
> > > > > +}
> > > > > +
> > > > > +static inline int anx7625_h_timing_reg_write(struct anx7625_data *ctx,
> > > > > + struct i2c_client *client,
> > > > > + u8 reg_addr, u16 val,
> > > > > + bool dsc_check) {
> > > > > + int ret;
> > > > > +
> > > > > + if (dsc_check && anx7625_dsc_check(ctx))
> > > > > + val = dsc_div(val);
> > > > > +
> > > > > + ret = anx7625_reg_write(ctx, client, reg_addr, val);
> > > > > + ret |= anx7625_reg_write(ctx, client, reg_addr + 1, val >>
> > > > > + 8);
> > > > > +
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > +static int anx7625_h_timing_write(struct anx7625_data *ctx,
> > > > > + struct i2c_client *client,
> > > > > + bool rx_h_timing) {
> > > > > + u16 htotal;
> > > > > + int ret;
> > > > > +
> > > > > + htotal = ctx->dt.hactive.min + ctx->dt.hfront_porch.min +
> > > > > + ctx->dt.hback_porch.min + ctx->dt.hsync_len.min;
> > > > > + /* Htotal */
> > > > > + ret = anx7625_h_timing_reg_write(ctx, client,
> > > > HORIZONTAL_TOTAL_PIXELS_L,
> > > > > + htotal, rx_h_timing);
> > > > > + /* Hactive */
> > > > > + ret |= anx7625_h_timing_reg_write(ctx, client,
> > > > HORIZONTAL_ACTIVE_PIXELS_L,
> > > > > + ctx->dt.hactive.min, rx_h_timing);
> > > > > + /* HFP */
> > > > > + ret |= anx7625_h_timing_reg_write(ctx, client,
> > > > HORIZONTAL_FRONT_PORCH_L,
> > > > > + ctx->dt.hfront_porch.min, rx_h_timing);
> > > > > + /* HWS */
> > > > > + ret |= anx7625_h_timing_reg_write(ctx, client,
> > > > HORIZONTAL_SYNC_WIDTH_L,
> > > > > + ctx->dt.hsync_len.min, rx_h_timing);
> > > > > + /* HBP */
> > > > > + ret |= anx7625_h_timing_reg_write(ctx, client,
> > > > HORIZONTAL_BACK_PORCH_L,
> > > > > + ctx->dt.hback_porch.min,
> > > > > + rx_h_timing);
> > > > > +
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > +static int anx7625_v_timing_write(struct anx7625_data *ctx,
> > > > > + struct i2c_client *client) {
> > > > > + int ret;
> > > > > +
> > > > > + /* Vactive */
> > > > > + ret = anx7625_reg_write(ctx, client, ACTIVE_LINES_L,
> > > > > + ctx->dt.vactive.min);
> > > > > + ret |= anx7625_reg_write(ctx, client, ACTIVE_LINES_H,
> > > > > + ctx->dt.vactive.min >> 8);
> > > > > + /* VFP */
> > > > > + ret |= anx7625_reg_write(ctx, client, VERTICAL_FRONT_PORCH,
> > > > > + ctx->dt.vfront_porch.min);
> > > > > + /* VWS */
> > > > > + ret |= anx7625_reg_write(ctx, client, VERTICAL_SYNC_WIDTH,
> > > > > + ctx->dt.vsync_len.min);
> > > > > + /* VBP */
> > > > > + ret |= anx7625_reg_write(ctx, client, VERTICAL_BACK_PORCH,
> > > > > + ctx->dt.vback_porch.min);
> > > > > +
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > +static int anx7625_set_dsc_params(struct anx7625_data *ctx) {
> > > > > + int ret, i;
> > > > > + u16 htotal, vtotal;
> > > > > +
> > > > > + if (!anx7625_dsc_check(ctx))
> > > > > + return 0;
> > > > > +
> > > > > + /* Video Horizontal timing */
> > > > > + ret = anx7625_h_timing_write(ctx, ctx->i2c.tx_p2_client,
> > > > > + false);
> > > > > +
> > > > > + /* Video Vertical timing */
> > > > > + ret |= anx7625_v_timing_write(ctx, ctx->i2c.tx_p2_client);
> > > > > +
> > > > > + /* Vtotal */
> > > > > + vtotal = ctx->dt.vactive.min + ctx->dt.vfront_porch.min +
> > > > > + ctx->dt.vback_porch.min + ctx->dt.vsync_len.min;
> > > > > + ret |= anx7625_reg_write(ctx, ctx->i2c.tx_p2_client, TOTAL_LINES_L,
> > > > > + vtotal);
> > > > > + ret |= anx7625_reg_write(ctx, ctx->i2c.tx_p2_client, TOTAL_LINES_H,
> > > > > + vtotal >> 8);
> > > > > + /* Htotal */
> > > > > + ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> TOTAL_PIXEL_L_7E,
> > > > > + htotal);
> > > > > + ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> TOTAL_PIXEL_H_7E,
> > > > > + htotal >> 8);
> > > > > + /* Hactive */
> > > > > + ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> > > > > + ACTIVE_PIXEL_L_7E, ctx->dt.hactive.min);
> > > > > + ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> > > > > + ACTIVE_PIXEL_H_7E, ctx->dt.hactive.min >> 8);
> > > > > + /* HFP */
> > > > > + ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> > > > > + HORIZON_FRONT_PORCH_L_7E,
> > > > > + ctx->dt.hfront_porch.min);
> > > > > + ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> > > > > + HORIZON_FRONT_PORCH_H_7E,
> > > > > + ctx->dt.hfront_porch.min >> 8);
> > > > > + /* HWS */
> > > > > + ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> > > > > + HORIZON_SYNC_WIDTH_L_7E,
> > > > > + ctx->dt.hsync_len.min);
> > > > > + ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> > > > > + HORIZON_SYNC_WIDTH_H_7E,
> > > > > + ctx->dt.hsync_len.min >> 8);
> > > > > + /* HBP */
> > > > > + ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> > > > > + HORIZON_BACK_PORCH_L_7E,
> > > > > + ctx->dt.hback_porch.min);
> > > > > + ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> > > > > + HORIZON_BACK_PORCH_H_7E,
> > > > > + ctx->dt.hback_porch.min >> 8);
> > > > > +
> > > > > + /* Config DSC decoder internal blank timing for decoder to start */
> > > > > + ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p1_client,
> > > > > + H_BLANK_L,
> > > > > + dsc_div(htotal - ctx->dt.hactive.min));
> > > > > + ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p1_client,
> > > > > + H_BLANK_H,
> > > > > + dsc_div(htotal -
> > > > > + ctx->dt.hactive.min)
> > > > > + >> 8);
> > > > > +
> > > > > + /* Compress ratio RATIO bit[7:6] */
> > > > > + ret |= anx7625_write_and(ctx, ctx->i2c.rx_p0_client, R_I2C_1, 0x3F);
> > > > > + ret |= anx7625_write_or(ctx, ctx->i2c.rx_p0_client, R_I2C_1,
> > > > > + (5 - DSC_COMPRESS_RATIO) << 6);
> > > > > + /*PPS table*/
> > > > > + for (i = 0; i < PPS_SIZE; i += PPS_BLOCK_SIZE)
> > > > > + ret |= anx7625_reg_block_write(ctx, ctx->i2c.rx_p2_client,
> > > > > + R_PPS_REG_0 + i, PPS_BLOCK_SIZE,
> > > > > +
> > > > > + &ctx->pps_table[i]);
> > > > > +
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > static int anx7625_dsi_video_timing_config(struct anx7625_data
> > > > > *ctx) {
> > > > > struct device *dev = ctx->dev;
> > > > > unsigned long m, n;
> > > > > - u16 htotal;
> > > > > int ret;
> > > > > u8 post_divider = 0;
> > > > >
> > > > > @@ -506,48 +656,12 @@ static int
> > > > > anx7625_dsi_video_timing_config(struct
> > > > anx7625_data *ctx)
> > > > > ret |= anx7625_write_or(ctx, ctx->i2c.rx_p1_client,
> > > > > MIPI_LANE_CTRL_0,
> > > > > ctx->pdata.mipi_lanes
> > > > > - 1);
> > > > >
> > > > > - /* Htotal */
> > > > > - htotal = ctx->dt.hactive.min + ctx->dt.hfront_porch.min +
> > > > > - ctx->dt.hback_porch.min + ctx->dt.hsync_len.min;
> > > > > - ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p2_client,
> > > > > - HORIZONTAL_TOTAL_PIXELS_L, htotal & 0xFF);
> > > > > - ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p2_client,
> > > > > - HORIZONTAL_TOTAL_PIXELS_H, htotal >> 8);
> > > > > - /* Hactive */
> > > > > - ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p2_client,
> > > > > - HORIZONTAL_ACTIVE_PIXELS_L, ctx->dt.hactive.min & 0xFF);
> > > > > - ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p2_client,
> > > > > - HORIZONTAL_ACTIVE_PIXELS_H, ctx->dt.hactive.min >> 8);
> > > > > - /* HFP */
> > > > > - ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p2_client,
> > > > > - HORIZONTAL_FRONT_PORCH_L, ctx->dt.hfront_porch.min);
> > > > > - ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p2_client,
> > > > > - HORIZONTAL_FRONT_PORCH_H,
> > > > > - ctx->dt.hfront_porch.min >> 8);
> > > > > - /* HWS */
> > > > > - ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p2_client,
> > > > > - HORIZONTAL_SYNC_WIDTH_L, ctx->dt.hsync_len.min);
> > > > > - ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p2_client,
> > > > > - HORIZONTAL_SYNC_WIDTH_H, ctx->dt.hsync_len.min >> 8);
> > > > > - /* HBP */
> > > > > - ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p2_client,
> > > > > - HORIZONTAL_BACK_PORCH_L, ctx->dt.hback_porch.min);
> > > > > - ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p2_client,
> > > > > - HORIZONTAL_BACK_PORCH_H, ctx->dt.hback_porch.min >>
> 8);
> > > > > - /* Vactive */
> > > > > - ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p2_client, ACTIVE_LINES_L,
> > > > > - ctx->dt.vactive.min);
> > > > > - ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p2_client, ACTIVE_LINES_H,
> > > > > - ctx->dt.vactive.min >> 8);
> > > > > - /* VFP */
> > > > > - ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p2_client,
> > > > > - VERTICAL_FRONT_PORCH, ctx->dt.vfront_porch.min);
> > > > > - /* VWS */
> > > > > - ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p2_client,
> > > > > - VERTICAL_SYNC_WIDTH, ctx->dt.vsync_len.min);
> > > > > - /* VBP */
> > > > > - ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p2_client,
> > > > > - VERTICAL_BACK_PORCH, ctx->dt.vback_porch.min);
> > > > > + /* Video Horizontal timing */
> > > > > + ret |= anx7625_h_timing_write(ctx, ctx->i2c.rx_p2_client,
> > > > > + true);
> > > > > +
> > > > > + /* Video Vertical timing */
> > > > > + ret |= anx7625_v_timing_write(ctx, ctx->i2c.rx_p2_client);
> > > > > +
> > > >
> > > > Please split this part into two commits: one refactoring timing
> > > > programming into two functions and another one introducing DSC support.
> > > > It is hard to review timing programming otherwise.
> > >
> > > OK
> > >
> > > >
> > > > > /* M value */
> > > > > ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p1_client,
> > > > > MIPI_PLL_M_NUM_23_16, (m >> 16) & 0xff);
> > > > > @@
> > > > > -663,9 +777,15 @@ static int anx7625_dsi_config(struct
> > > > > anx7625_data
> > > > > *ctx)
> > > > >
> > > > > DRM_DEV_DEBUG_DRIVER(dev, "config dsi.\n");
> > > > >
> > > > > - /* DSC disable */
> > > > > - ret = anx7625_write_and(ctx, ctx->i2c.rx_p0_client,
> > > > > - R_DSC_CTRL_0, ~DSC_EN);
> > > > > + ret = anx7625_set_dsc_params(ctx);
> > > > > + if (anx7625_dsc_check(ctx))
> > > > > + /* DSC enable */
> > > > > + ret |= anx7625_write_or(ctx, ctx->i2c.rx_p0_client,
> > > > > + R_DSC_CTRL_0, DSC_EN);
> > > > > + else
> > > > > + /* DSC disable */
> > > > > + ret |= anx7625_write_and(ctx, ctx->i2c.rx_p0_client,
> > > > > + R_DSC_CTRL_0, ~DSC_EN);
> > > > >
> > > > > ret |= anx7625_api_dsi_config(ctx);
> > > > >
> > > > > @@ -2083,6 +2203,7 @@ static int anx7625_setup_dsi_device(struct
> > > > anx7625_data *ctx)
> > > > > MIPI_DSI_MODE_VIDEO_HSE |
> > > > > MIPI_DSI_HS_PKT_END_ALIGNED;
> > > > >
> > > > > + dsi->dsc = &ctx->dsc;
> > > > > ctx->dsi = dsi;
> > > > >
> > > > > return 0;
> > > > > @@ -2187,19 +2308,69 @@ anx7625_bridge_mode_valid(struct
> > > > > drm_bridge
> > > > *bridge,
> > > > > struct device *dev = ctx->dev;
> > > > >
> > > > > DRM_DEV_DEBUG_DRIVER(dev, "drm mode checking\n");
> > > > > + if (mode->clock > SUPPORT_PIXEL_CLOCK)
> > > > > + return MODE_CLOCK_HIGH;
> > > > > +
> > > > > + if (mode->clock < SUPPORT_MIN_PIXEL_CLOCK)
> > > > > + return MODE_CLOCK_LOW;
> > > > >
> > > > > - /* Max 1200p at 5.4 Ghz, one lane, pixel clock 300M */
> > > > > - if (mode->clock > SUPPORT_PIXEL_CLOCK) {
> > > > > - DRM_DEV_DEBUG_DRIVER(dev,
> > > > > - "drm mode invalid, pixelclock too high.\n");
> > > >
> > > > Any reason for dropping debug message?
> > >
> > > I'll keep this message.
> > >
> > > >
> > > > > + /*
> > > > > + * If hdisplay cannot be divided by DSC compress ratio, then display
> > > > > + * will have overlap/shift issue
> > > > > + */
> > > > > + if (mode->clock > DSC_PIXEL_CLOCK &&
> > > > > + (mode->hdisplay % DSC_COMPRESS_RATIO != 0))
> > > >
> > > >
> > > > ... and there still no check that the DSI host supports generating
> > > > DSC data. Nor there is an atomic_check() that will check if the mode can be
> enabled.
> > >
> > > I cannot know whether the DSI host supports DSC or not. Anx7625
> > > driver force enable DSC feature if pixel clock higher than DSC_PIXEL_CLOCK.
> >
> > This is called upstream. Please work on necessary API changes rather
> > than claiming that it is not possible. Enabling DSC support when DSC
> > is not handled by the DSI host is not acceptable.
> >
> > Few semi-random ideas:
> > - Add DSC checking callbacks to struct mipi_dsi_host_ops. Assume that
> > DSC support is not available if callback is not present. The callback
> > should accept struct drm_dsc_config and populate const and RC params.
> >
> > - Add DSC-related flags to struct mipi_dsi_host, describing DSC feature
> > availability. Make anx7625 check those flags.
>
> ...
>
> - Extend input_/output_bus_format checks to also cover DSC.
OK, I'll discuss it with MTK DSI host engineer.
>
> --
> With best wishes
> Dmitry
Powered by blists - more mailing lists