| 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: <z5uepwsk6t6j5zlkrfrkcuodcmx477b7wr7smziwj4nf3avhv2@p7bqiun4yxb7>
Date: Mon, 31 Mar 2025 10:28:00 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: Xin Ji <xji@...logixsemi.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
On Mon, Mar 31, 2025 at 05:44:58AM +0000, Xin Ji wrote:
> > > 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.
>
> >
> > > return MODE_CLOCK_HIGH;
> > > - }
> > >
> > > DRM_DEV_DEBUG_DRIVER(dev, "drm mode valid.\n");
> > >
> > > return MODE_OK;
> > > }
> > >
> > > +static void anx7625_dsc_enable(struct anx7625_data *ctx, bool en) {
> > > + int ret;
> > > + struct device *dev = ctx->dev;
> > > +
> > > + if (en) {
> > > + ctx->dsc.dsc_version_major = 1;
> > > + ctx->dsc.dsc_version_minor = 1;
> > > + ctx->dsc.slice_height = 8;
> > > + ctx->dsc.slice_width = ctx->dt.hactive.min / DSC_SLICE_NUM;
> > > + ctx->dsc.slice_count = DSC_SLICE_NUM;
> > > + ctx->dsc.bits_per_component = 8;
> > > + ctx->dsc.bits_per_pixel = 8 << 4; /* 4 fractional bits */
> > > + ctx->dsc.block_pred_enable = true;
> > > + ctx->dsc.native_420 = false;
> > > + ctx->dsc.native_422 = false;
> > > + ctx->dsc.simple_422 = false;
> > > + ctx->dsc.vbr_enable = false;
> > > + ctx->dsc.convert_rgb = 1;
> > > + ctx->dsc.vbr_enable = 0;
> >
> > Aren't those 'false' and '0' defaults? If so, you don't need to write those fields.
> Ok
>
> >
> > > +
> > > + drm_dsc_set_rc_buf_thresh(&ctx->dsc);
> > > + drm_dsc_set_const_params(&ctx->dsc);
> > > +
> > > + ctx->dsc.initial_scale_value = drm_dsc_initial_scale_value(&ctx->dsc);
> > > + ctx->dsc.line_buf_depth = ctx->dsc.bits_per_component + 1;
> > > + ret = drm_dsc_setup_rc_params(&ctx->dsc, DRM_DSC_1_2_444);
> > > + if (ret < 0)
> > > + dev_warn(dev, "drm_dsc_setup_rc_params ret
> > > + %d\n", ret);
> > > +
> > > + ret = drm_dsc_compute_rc_parameters(&ctx->dsc);
BTW: these calls belong to the DSI host driver. See msm/dsi/dsi_host.c
and DSC-enabled panel drivers.
> > > + if (ret)
> > > + dev_warn(dev, "drm dsc compute rc parameters
> > > + failed ret %d\n", ret);
I think this should become an error rather than a warning.
> > > +
> > > + drm_dsc_pps_payload_pack((struct drm_dsc_picture_parameter_set
> > *)&ctx->pps_table,
> > > + &ctx->dsc);
> > > + dev_dbg(dev, "anx7625 enable dsc\n");
> > > + } else {
> > > + ctx->dsc.dsc_version_major = 0;
> > > + ctx->dsc.dsc_version_minor = 0;
> > > + dev_dbg(dev, "anx7625 disable dsc\n");
> > > + }
> > > +}
> > > +
--
With best wishes
Dmitry
Powered by blists - more mailing lists