> -----Original Message-----
> From: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> Sent: Thursday, February 13, 2025 9:04 PM
> To: Xin Ji <xji@xxxxxxxxxxxxxxxx>
> Cc: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>; Neil Armstrong
> <neil.armstrong@xxxxxxxxxx>; Robert Foss <rfoss@xxxxxxxxxx>; Laurent Pinchart
> <Laurent.pinchart@xxxxxxxxxxxxxxxx>; Jonas Karlman <jonas@xxxxxxxxx>;
> Jernej Skrabec <jernej.skrabec@xxxxxxxxx>; Maarten Lankhorst
> <maarten.lankhorst@xxxxxxxxxxxxxxx>; Maxime Ripard <mripard@xxxxxxxxxx>;
> Thomas Zimmermann <tzimmermann@xxxxxxx>; David Airlie
> <airlied@xxxxxxxxx>; Simona Vetter <simona@xxxxxxxx>; Bernie Liang
> <bliang@xxxxxxxxxxxxxxxx>; Qilin Wen <qwen@xxxxxxxxxxxxxxxx>;
> treapking@xxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] drm/bridge:anx7625: Enable DSC feature
>
>
> On Thu, Feb 13, 2025 at 08:33:30PM +0800, Xin Ji wrote:
> > As anx7625 MIPI RX bandwidth(maximum 1.5Gbps per lane) and internal
> > pixel clock(maximum 300M) limitation. Anx7625 must enable DSC feature
> > while MIPI source want to output 4K30 resolution.
>
> This commit message is pretty hard to read and understand for a non-native
> speaker. Please consider rewriting it so that it is easier to understand it.
>
Thanks for the review, sorry about that, I'll rewriting the commit message
> >
> > Signed-off-by: Xin Ji <xji@xxxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/bridge/analogix/anx7625.c | 300
> > ++++++++++++++++++---- drivers/gpu/drm/bridge/analogix/anx7625.h |
> > 32 +++
> > 2 files changed, 284 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > index 4be34d5c7a3b..7d86ab02f71c 100644
> > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > @@ -22,6 +22,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,6
> > +477,138 @@ static int anx7625_set_k_value(struct anx7625_data *ctx)
> > MIPI_DIGITAL_ADJ_1, 0x3D); }
> >
> > +static int anx7625_set_dsc_params(struct anx7625_data *ctx) {
> > + int ret, i;
> > + u16 htotal, vtotal;
> > +
> > + if (!ctx->dsc_en)
> > + return 0;
> > +
> > + /* 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.tx_p2_client,
> > + HORIZONTAL_TOTAL_PIXELS_L, htotal);
> > + ret |= anx7625_reg_write(ctx, ctx->i2c.tx_p2_client,
> > + HORIZONTAL_TOTAL_PIXELS_H, htotal >> 8);
> > + /* Hactive */
> > + ret |= anx7625_reg_write(ctx, ctx->i2c.tx_p2_client,
> > + HORIZONTAL_ACTIVE_PIXELS_L,
> > + ctx->dt.hactive.min);
> > + ret |= anx7625_reg_write(ctx, ctx->i2c.tx_p2_client,
> > + HORIZONTAL_ACTIVE_PIXELS_H,
> > + ctx->dt.hactive.min >> 8);
> > + /* HFP */
> > + ret |= anx7625_reg_write(ctx, ctx->i2c.tx_p2_client,
> > + HORIZONTAL_FRONT_PORCH_L,
> > + ctx->dt.hfront_porch.min);
> > + ret |= anx7625_reg_write(ctx, ctx->i2c.tx_p2_client,
> > + HORIZONTAL_FRONT_PORCH_H,
> > + ctx->dt.hfront_porch.min >> 8);
> > + /* HWS */
> > + ret |= anx7625_reg_write(ctx, ctx->i2c.tx_p2_client,
> > + HORIZONTAL_SYNC_WIDTH_L,
> > + ctx->dt.hsync_len.min);
> > + ret |= anx7625_reg_write(ctx, ctx->i2c.tx_p2_client,
> > + HORIZONTAL_SYNC_WIDTH_H,
> > + ctx->dt.hsync_len.min >> 8);
> > + /* HBP */
> > + ret |= anx7625_reg_write(ctx, ctx->i2c.tx_p2_client,
> > + HORIZONTAL_BACK_PORCH_L,
> > + ctx->dt.hback_porch.min);
> > + ret |= anx7625_reg_write(ctx, ctx->i2c.tx_p2_client,
> > + HORIZONTAL_BACK_PORCH_H,
> > + ctx->dt.hback_porch.min >> 8);
> > + /* 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);
> > + /* Vactive */
> > + ret |= anx7625_reg_write(ctx, ctx->i2c.tx_p2_client, ACTIVE_LINES_L,
> > + ctx->dt.vactive.min);
> > + ret |= anx7625_reg_write(ctx, ctx->i2c.tx_p2_client, ACTIVE_LINES_H,
> > + ctx->dt.vactive.min >> 8);
> > + /* VFP */
> > + ret |= anx7625_reg_write(ctx, ctx->i2c.tx_p2_client,
> > + VERTICAL_FRONT_PORCH, ctx->dt.vfront_porch.min);
> > + /* VWS */
> > + ret |= anx7625_reg_write(ctx, ctx->i2c.tx_p2_client,
> > + VERTICAL_SYNC_WIDTH, ctx->dt.vsync_len.min);
> > + /* VBP */
> > + ret |= anx7625_reg_write(ctx, ctx->i2c.tx_p2_client,
> > + VERTICAL_BACK_PORCH,
> > + ctx->dt.vback_porch.min);
>
> This code mostly duplicates anx7625_dsi_video_timing_config() with the I2C
> client being the only difference. Please consider extracting a common helper to
> write the timings.
It is hard to extracting a common helper, they are used different I2C slave address
and timing register need divided by 3 if DSC enabled.
>
> > +
> > + /* 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_timing_write(struct anx7625_data *ctx,
> > + struct i2c_client *client,
> > + u8 reg_addr, u16 reg_val, bool
> > +high_byte) {
> > + u8 reg_data;
> > +
> > + if (ctx->dsc_en)
> > + reg_val = dsc_div(reg_val);
> > +
> > + reg_data = (high_byte ? (reg_val >> 8) : reg_val) & 0xFF;
> > +
> > + return anx7625_reg_write(ctx, client, reg_addr, reg_data); }
>
> Ugh, no. Calculate htotal in the calling function and call
> anx7625_reg_write() as you were doing that before.
>
anx7625_timing_write() is a common function, it was called many times,
so we cannot calculate htotal in it.
> > +
> > static int anx7625_dsi_video_timing_config(struct anx7625_data *ctx)
> > {
> > struct device *dev = ctx->dev;
> > @@ -506,34 +639,33 @@ 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);
> > + ctx->dt.hback_porch.min + ctx->dt.hsync_len.min;
> > + /* Htotal */
> > + ret |= anx7625_timing_write(ctx, ctx->i2c.rx_p2_client,
> > + HORIZONTAL_TOTAL_PIXELS_L, htotal, false);
> > + ret |= anx7625_timing_write(ctx, ctx->i2c.rx_p2_client,
> > + HORIZONTAL_TOTAL_PIXELS_H, htotal, true);
> > /* 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);
> > + ret |= anx7625_timing_write(ctx, ctx->i2c.rx_p2_client,
> > + HORIZONTAL_ACTIVE_PIXELS_L, ctx->dt.hactive.min, false);
> > + ret |= anx7625_timing_write(ctx, ctx->i2c.rx_p2_client,
> > + HORIZONTAL_ACTIVE_PIXELS_H, ctx->dt.hactive.min,
> > + true);
> > /* 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);
> > + ret |= anx7625_timing_write(ctx, ctx->i2c.rx_p2_client,
> > + HORIZONTAL_FRONT_PORCH_L, ctx->dt.hfront_porch.min, false);
> > + ret |= anx7625_timing_write(ctx, ctx->i2c.rx_p2_client,
> > + HORIZONTAL_FRONT_PORCH_H,
> > + ctx->dt.hfront_porch.min, true);
>
> Are porches also compressed? I think blanking signal timings are transferred as is,
> without compression.
Porches not be compressed, anx7625 internal digital block will multiply by 3 while enable
DSC feature, so the register must divided by 3.
>
> > /* 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);
> > + ret |= anx7625_timing_write(ctx, ctx->i2c.rx_p2_client,
> > + HORIZONTAL_SYNC_WIDTH_L, ctx->dt.hsync_len.min, false);
> > + ret |= anx7625_timing_write(ctx, ctx->i2c.rx_p2_client,
> > + HORIZONTAL_SYNC_WIDTH_H, ctx->dt.hsync_len.min,
> > + true);
> > /* 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);
> > + ret |= anx7625_timing_write(ctx, ctx->i2c.rx_p2_client,
> > + HORIZONTAL_BACK_PORCH_L, ctx->dt.hback_porch.min, false);
> > + ret |= anx7625_timing_write(ctx, ctx->i2c.rx_p2_client,
> > + HORIZONTAL_BACK_PORCH_H,
> > + ctx->dt.hback_porch.min, true);
> > /* Vactive */
> > ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p2_client, ACTIVE_LINES_L,
> > ctx->dt.vactive.min); @@ -663,9 +795,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 (ctx->dsc_en)
> > + /* 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 +2221,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;
> > @@ -2186,20 +2325,68 @@ anx7625_bridge_mode_valid(struct drm_bridge
> *bridge,
> > struct anx7625_data *ctx = bridge_to_anx7625(bridge);
> > struct device *dev = ctx->dev;
> >
> > - DRM_DEV_DEBUG_DRIVER(dev, "drm mode checking\n");
> > + dev_dbg(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");
> > + if (mode->clock > DSC_PIXEL_CLOCK && (mode->hdisplay % 3 != 0))
>
> Magic number 3. Also is it actually required? I think DSC standard has no such
> requirement.
This is anx7625 limitation, if mode->hdisplay cannot be divided by 3, then display will have
overlap/shift issue.
>
> > return MODE_CLOCK_HIGH;
> > - }
> >
> > - DRM_DEV_DEBUG_DRIVER(dev, "drm mode valid.\n");
> > + dev_dbg(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;
> > +
> > + ctx->dsc_en = en;
> > +
> > + 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.rc_model_size = DSC_RC_MODEL_SIZE_CONST;
> > + ctx->dsc.pic_width = ctx->dt.hactive.min;
> > + ctx->dsc.pic_height = ctx->dt.vactive.min;
> > + ctx->dsc.convert_rgb = 1;
> > + ctx->dsc.vbr_enable = 0;
>
> A lot of this params should be coming from the sink device. You have to get
> them from the DPCD.
These parameter is just MIPI source and anx7625 and used only for DSC feature,
Anx7625 is bridge IC, sink monitor only receive normal DP signal from anx7625,
sink device didn't know DSC information, no need read DPCD.
>
> > +
> > + 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);
> > +
> > + drm_dsc_compute_rc_parameters(&ctx->dsc);
>
> You have ignored return value. Please handle the function returning an error.
OK
>
> > +
> > + 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");
> > + }
> > +}
> > +
> > static void anx7625_bridge_mode_set(struct drm_bridge *bridge,
> > const struct drm_display_mode *old_mode,
> > const struct drm_display_mode *mode)
> > @@ -2244,6 +2431,11 @@ static void anx7625_bridge_mode_set(struct
> drm_bridge *bridge,
> > DRM_DEV_DEBUG_DRIVER(dev, "vsync_end(%d),vtotal(%d).\n",
> > mode->vsync_end,
> > mode->vtotal);
> > +
> > + if (mode->clock > DSC_PIXEL_CLOCK)
>
> What if the sink doesn't support DSC? The decision whether to enable DSC or not
> should be happening in the atomic_check(). Likewise mode_valid should be able
> to check if your bridge and the sink can agree on DSC params and reject modes if
> they can not.
Anx7625 is bridge IC, it receive MIPI data and DP output, DSC only exist between SOC
and Anx7625, sink monitor only receive normal DP signal from anx7625,
there is no compression at DP output, so no need to check sink device.
>
> > + anx7625_dsc_enable(ctx, true);
> > + else
> > + anx7625_dsc_enable(ctx, false);
> > }
> >
> > static bool anx7625_bridge_mode_fixup(struct drm_bridge *bridge, @@
> > -2258,26 +2450,33 @@ static bool anx7625_bridge_mode_fixup(struct
> > drm_bridge *bridge,
> >
> > DRM_DEV_DEBUG_DRIVER(dev, "drm mode fixup set\n");
> >
> > - /* No need fixup for external monitor */
> > - if (!ctx->pdata.panel_bridge)
> > - return true;
> > -
> > hsync = mode->hsync_end - mode->hsync_start;
> > hfp = mode->hsync_start - mode->hdisplay;
> > hbp = mode->htotal - mode->hsync_end;
> > hblanking = mode->htotal - mode->hdisplay;
> >
> > - DRM_DEV_DEBUG_DRIVER(dev, "before mode fixup\n");
> > - DRM_DEV_DEBUG_DRIVER(dev, "hsync(%d), hfp(%d), hbp(%d),
> clock(%d)\n",
> > - hsync, hfp, hbp, adj->clock);
> > - DRM_DEV_DEBUG_DRIVER(dev, "hsync_start(%d), hsync_end(%d),
> htot(%d)\n",
> > - adj->hsync_start, adj->hsync_end, adj->htotal);
> > -
> > + dev_dbg(dev, "before mode fixup\n");
> > + dev_dbg(dev, "hsync(%d), hfp(%d), hbp(%d), clock(%d)\n",
> > + hsync, hfp, hbp, adj->clock);
> > + dev_dbg(dev, "hsync_start(%d), hsync_end(%d), htot(%d)\n",
> > + adj->hsync_start, adj->hsync_end, adj->htotal);
>
> No. Please use drm_dbg_driver(), but not dev_dbg(). And anyway, this should go
> to a separate commit.
OK, I'll not change it in this patch.
>
> > adj_hfp = hfp;
> > adj_hsync = hsync;
> > adj_hbp = hbp;
> > adj_hblanking = hblanking;
> >
> > + if (mode->clock > DSC_PIXEL_CLOCK) {
> > + adj_hsync = DSC_HSYNC_LEN;
> > + adj_hfp = DSC_HFP_LEN;
> > + adj_hbp = DSC_HBP_LEN;
> > + vref = (u64)adj->clock * 1000 * 1000 / (adj->htotal * adj->vtotal);
> > + goto calculate_timing;
> > + }
> > +
> > + /* No need fixup for external monitor */
> > + if (!ctx->pdata.panel_bridge)
> > + return true;
> > +
> > /* HFP needs to be even */
> > if (hfp & 0x1) {
> > adj_hfp += 1;
> > @@ -2349,16 +2548,21 @@ static bool anx7625_bridge_mode_fixup(struct
> drm_bridge *bridge,
> > adj_hfp -= delta_adj;
> > }
> >
> > - DRM_DEV_DEBUG_DRIVER(dev, "after mode fixup\n");
> > - DRM_DEV_DEBUG_DRIVER(dev, "hsync(%d), hfp(%d), hbp(%d),
> clock(%d)\n",
> > - adj_hsync, adj_hfp, adj_hbp, adj->clock);
> > +calculate_timing:
> > +
> > + dev_dbg(dev, "after mode fixup\n");
> > + dev_dbg(dev, "hsync(%d), hfp(%d), hbp(%d), clock(%d)\n",
> > + adj_hsync, adj_hfp, adj_hbp, adj->clock);
> >
> > /* Reconstruct timing */
> > adj->hsync_start = adj->hdisplay + adj_hfp;
> > adj->hsync_end = adj->hsync_start + adj_hsync;
> > adj->htotal = adj->hsync_end + adj_hbp;
> > - DRM_DEV_DEBUG_DRIVER(dev, "hsync_start(%d), hsync_end(%d),
> htot(%d)\n",
> > - adj->hsync_start, adj->hsync_end, adj->htotal);
> > + if (mode->clock > DSC_PIXEL_CLOCK)
> > + adj->clock = (u64)vref * adj->htotal * adj->vtotal /
> > + 1000 / 1000;
> > +
> > + dev_dbg(dev, "hsync_start(%d), hsync_end(%d), htot(%d), clock(%d)\n",
> > + adj->hsync_start, adj->hsync_end, adj->htotal,
> > + adj->clock);
> >
> > return true;
> > }
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h
> > b/drivers/gpu/drm/bridge/analogix/anx7625.h
> > index eb5580f1ab2f..db931f5800b1 100644
> > --- a/drivers/gpu/drm/bridge/analogix/anx7625.h
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
> > @@ -149,6 +149,8 @@
> > #define HFP_HBP_DEF ((HBLANKING_MIN - SYNC_LEN_DEF) / 2)
> > #define VIDEO_CONTROL_0 0x08
> >
> > +#define TOTAL_LINES_L 0x12
> > +#define TOTAL_LINES_H 0x13
> > #define ACTIVE_LINES_L 0x14
> > #define ACTIVE_LINES_H 0x15 /* Bit[7:6] are reserved */
> > #define VERTICAL_FRONT_PORCH 0x16
> > @@ -166,6 +168,32 @@
> > #define HORIZONTAL_BACK_PORCH_L 0x21
> > #define HORIZONTAL_BACK_PORCH_H 0x22 /* Bit[7:4] are reserved */
> >
> > +#define H_BLANK_L 0x3E
> > +#define H_BLANK_H 0x3F
> > +#define DSC_COMPRESS_RATIO 3
> > +#define dsc_div(X) ((X) / DSC_COMPRESS_RATIO)
>
> This assumes 1:3 ratio. Does anx7625 support only 8bpp / 8bpc mode? Or does
> 1:3 come from some other ratio?
We only tested 1:3 compression ratio, 24bpp compress to 8bpp.
>
> > +#define DSC_SLICE_NUM 2
> > +#define DSC_PIXEL_CLOCK 250000
> > +#define DSC_HSYNC_LEN 90
> > +#define DSC_HFP_LEN 177
> > +#define DSC_HBP_LEN 297
> > +
> > +#define TOTAL_PIXEL_L_7E 0x50
> > +#define TOTAL_PIXEL_H_7E 0x51 /* bit[7:6] are reserved */
> > +#define ACTIVE_PIXEL_L_7E 0x52
> > +#define ACTIVE_PIXEL_H_7E 0x53 /* bit[7:6] are reserved */
> > +#define HORIZON_FRONT_PORCH_L_7E 0x54
> > +#define HORIZON_FRONT_PORCH_H_7E 0x55
> > +#define HORIZON_SYNC_WIDTH_L_7E 0x56
> > +#define HORIZON_SYNC_WIDTH_H_7E 0x57
> > +#define HORIZON_BACK_PORCH_L_7E 0x58
> > +#define HORIZON_BACK_PORCH_H_7E 0x59
> > +
> > +#define PPS_SIZE 128
> > +#define PPS_BLOCK_SIZE 32
> > +#define R_PPS_REG_0 0x80
> > +#define R_I2C_1 0x81
> > +
> > /******** END of I2C Address 0x72 *********/
> >
> >
> /***************************************************************/
> > @@ -415,6 +443,7 @@ enum audio_wd_len {
> > #define MAX_EDID_BLOCK 3
> > #define EDID_TRY_CNT 3
> > #define SUPPORT_PIXEL_CLOCK 300000
> > +#define SUPPORT_MIN_PIXEL_CLOCK 50000
> >
> > /***************** Display End *****************/
> >
> > @@ -454,6 +483,7 @@ struct anx7625_data {
> > int hpd_high_cnt;
> > int dp_en;
> > int hdcp_cp;
> > + bool dsc_en;
> > /* Lock for work queue */
> > struct mutex lock;
> > struct device *dev;
> > @@ -479,6 +509,8 @@ struct anx7625_data {
> > struct drm_connector *connector;
> > struct mipi_dsi_device *dsi;
> > struct drm_dp_aux aux;
> > + struct drm_dsc_config dsc;
> > + char pps_table[PPS_SIZE];
>
> pps_table and drm_dsc_config can vary depending on the mode and connected
> sink. THey should be a part of the atomic state rather than a long-living
> anx7625_data. So does dsc_en.
No need to check sink monitor, because DSC only exist between SOC and Anx7625,
sink monitor only receive normal DP signal from anx7625. Anx7625 is responsible for
decompression.
>
> > };
> >
> > #endif /* __ANX7625_H__ */
> > --
> > 2.25.1
> >
>
> --
> With best wishes
> Dmitry
