| 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:
<BY5PR04MB673907E1BDED253B27C0C71FC7AD2@BY5PR04MB6739.namprd04.prod.outlook.com>
Date: Mon, 31 Mar 2025 03:04:07 +0000
From: Xin Ji <xji@...logixsemi.com>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
CC: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>, 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@...gle.com"
<treapking@...gle.com>, "dri-devel@...ts.freedesktop.org"
<dri-devel@...ts.freedesktop.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] drm/bridge:anx7625: Enable DSC feature
> > > > > > > > > > > > > > > > From: Dmitry Baryshkov
> > > > > > > > > > > > > > > > <dmitry.baryshkov@...aro.org>
> > > > > > > > > > > > > > > > Sent: Thursday, February 13, 2025 9:04 PM
> > > > > > > > > > > > > > > > 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@...gle.com;
> > > > > > > > > > > > > > > > dri-devel@...ts.freedesktop.org; linux-
> > > > > > > > > > > > > > > > kernel@...r.kernel.org
> > > > > > > > > > > > > > > > Subject: Re: [PATCH] drm/bridge:anx7625: Enable
> > > > > > > > > > > > > > > > DSC feature
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > PLease remove such splats, use something more sensible.
> > > > > > > > > > > > > OK, I'll change the subject
> > > > > > > > > > > >
> > > > > > > > > > > > It's not about the subject. Compare just "ABC DEF wrote:"
> > > > > > > > > > > > and your quatation header.
> > > > > > > > > > > Sorry, these message is automatically added by Outlook, I'll
> > > remove it.
> > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > 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@...logixsemi.com>
> > > > > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > > > > 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.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I2C client can be a parameter. Also please comment
> > > > > > > > > > > > > > whether
> > > > > > > > > > > > > > /3 is because of bpp/bpc ratio or due to some other
> reason?
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Does anx7625 support any mode other than 8bpp / 8bpc?
> > > > > > > > > > > > > I'll try to make a common function, anx7625 mipi only
> > > > > > > > > > > > > support RGB888. DSC
> > > > > > > > > > > > 8bpp.
> > > > > > > > > > > > > /3 is because of DSC ration 1:3.
> > > > > > > > > > > >
> > > > > > > > > > > > Please add this to the commit message, add a comment next
> > > > > > > > > > > > to the corresponding #define and use it instead of /3 all over
> the
> > > > > place.
> > > > > > > > > > > OK
> > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > +
> > > > > > > > > > > > > > > > > + /* 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.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On the caller side. I'd suggest to inline
> > > > > > > > > > > > > > anx7625_timing_write() to make the code more explicit.
> > > > > > > > > > > > > OK.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > +
> > > > > > > > > > > > > > > > > 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.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Ack
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > /* 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.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Ack, add a comment or extend a commit message please.
> > > > > > > > > > > > > OK, I'll add a comment
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > 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;
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > This one is default, please drop.
> > > > > > > > > > > > > OK
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > + ctx->dsc.pic_width = ctx->dt.hactive.min;
> > > > > > > > > > > > > > > > > + ctx->dsc.pic_height =
> > > > > > > > > > > > > > > > > + ctx->dt.vactive.min;
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > These two are set on the DSC encoder side.
> > > > > > > > > > > > > OK, I'll remove it
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > + 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.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Thanks. From you commit message it is impossible to
> > > > > > > > > > > > > > understand whether DSC happens between the host and
> > > > > > > > > > > > > > your bridge or between the bridge and the monitor.
> > > > > > > > > > > > > I'll add more detail in commit message
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > +
> > > > > > > > > > > > > > > > > + 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
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > All these functions are to be called on the DSC
> > > > > > > > > > > > > > encoder side as it has to be able to infer DSC params.
> > > > > > > > > > > > >
> > > > > > > > > > > > > I don't know, we co-work with MTK encoder, they need us
> > > > > > > > > > > > > to initial dsc data structure
> > > > > > > > > > > >
> > > > > > > > > > > > As I wrote, encoder should be able to influence some of
> > > > > > > > > > > > those params. As such, DSC decoder should provide some
> > > > > > > > > > > > values, then DSC encoder's .pre_enable should be able to
> update
> > > > > those values (e.g.
> > > > > > > > > > > > by setting .pic_width and .pic_height and/or disabling some
> of
> > > flags).
> > > > > > > > > > > > Then the DSC encoder should fill the rest of the params by
> > > > > > > > > > > > calling all these
> > > > > > > > > > functions. Consider how drm/msm/dsi handles it.
> > > > > > > > > > > OK, I'll check it.
> > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > +
> > > > > > > > > > > > > > > > > + 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.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Well. Then the same question applies in the other
> direction:
> > > > > > > > > > > > > > what if the host doesnt' support DSC?
> > > > > > > > > > > > > If SOC host doesn't support DSC, then cannot display
> > > > > > > > > > > > > 4K30, currently, only debug with MTK DSI encoder.
> > > > > > > > > > > >
> > > > > > > > > > > > The bridge is a generic driver. So while you test with
> > > > > > > > > > > > Mediatek platforms, you still have to think about a
> > > > > > > > > > > > generic case and add necessary
> > > > > > > > > > checks.
> > > > > > > > > > > OK, I'll check it.
> > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > + 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.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > What are hardware limitations?
> > > > > > > > > > > > > Anx7625 MIPI only support RGB888 24bpp. With DSC 3:1
> > > > > > > > > > > > > compression, MIPI TX output 8bpp.
> > > > > > > > > > > >
> > > > > > > > > > > > As I wrote, a comment please.
> > > > > > > > > > > OK
> > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > +#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.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > The same way, they depend on the selected video mode,
> > > don't
> > > > > they?
> > > > > > > > > > > > > Actually it depends on pixel clock, if it is more than
> > > > > > > > > > > > > 250M, then need to do
> > > > > > > > > > DSC.
> > > > > > > > > > > >
> > > > > > > > > > > > Yes. So you should check if you can enable DSC in
> > > > > > > > > > > > .atomic_check() and set dsc_en correspondingly. At the
> > > > > > > > > > > > same time in
> > > > > > > > > > > > .atomic_check() you can not modify anx7625_data as it is a
> > > > > > > > > > > > check time, commit can be rejected or it can be a test-
> > > > > > > > > > > > only check. So dsc_en should go to
> > > > > > > > > > drm_bridge_state-wrapping structure.
> > > > > > > > > > > >
> > > > > > > > > > > > At the same time I don't see a value in storing the packed
> > > > > > > > > > > > PPS table, it is a short- lived data.
> > > > > > > > > > > Do you mean we can maintain dsc_en flag for each
> > > > > > > > > > > timing/resolution in drm_bridge_state structure, and set it
> > > > > > > > > > > in .atomic_check(), and check the drm_bridge_state's dsc_en
> > > > > > > > > > > in .atomic_enable()? I don't know how to
> > > > > > > > > > make a drm_bridge_state-wrapping structure.
> > > > > > > > > >
> > > > > > > > > > Ideally yes. You should define
> > > > > > > > > > struct anx7625_bridge_state {
> > > > > > > > > > struct drm_bridge_state base;
> > > > > > > > > > bool dsc_en;
> > > > > > > > > > };
> > > > > > > > > >
> > > > > > > > > > Then define your own versions of atomic_reset and
> > > > > > > > > > .atomic_duplicate_state callbacks.
> > > > > > > > > >
> > > > > > > > > > Note, we currently don't have a way to check that the DSI host
> > > > > > > > > > actually supports DSC, you have to invent it.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > OK, got it, I'll try it and discuss with related people, thanks!
> > > > > > > > >
> > > > > > > >
> > > > > > > > Hi Dmitry,
> > > > > > > > 1. I double confirmed with Mediatek encoder, they didn't have such
> > > > > > > > a capability which can tell us whether it can support DSC or not.
> > > > > > >
> > > > > > > I'm not sure, what do you mean here. I'm talking a driver interface,
> > > > > > > not about the Mediatek encoder on its own. The ANX7625 can be
> used
> > > > > > > with any DSI host, including one of the hosts which do not support
> > > > > > > DSC. The
> > > > > > > ANX7625 driver needs to negotate usage of the DSC with the
> underlying
> > > > > bridge.
> > > > > > >
> > > > > > > > 2. I tried to redefine .atomic_duplicate_state, and driver will
> > > > > > > > save dsc_en in new bridge state in .atomic_check(), but I found 2
> issue
> > > > > > > > a). As MTK driver need get DSC information before
> > > > > > > > .atomic_pre_enable(), driver must check dsc_en and set DSC
> > > > > > > > parameters in .mode_set(). But I cannot get
> > > > > > >
> > > > > > > By the 'set' do you mean 'calculate' or 'program to the hardware'?
> > > > > > >
> > > > > > > > drm_bridge_state data structure int .mode_set() interface.
> > > > > > >
> > > > > > > Don't use .mode_set() at all. Calculate parameters in
> > > > > > > .atomic_check() and then write them in .atomic_enable(). You should
> > > > > > > be able to fold
> > > > > > > .mode_set() into .atomic_enable() anyway.
> > > > > > Hi, MTK DSI encoder check DSC enable flag in DSI's .atomic_enable()
> > > > > > interface, but
> > > > >
> > > > > Could you please provide me with a pointer to the source code?
> > > > >
> > > > > > upper layer call MTK dsi encoder .atomic_enable() is early than
> > > > > > calling bridge's
> > > > > > .atomic_pre_enable() interface. So, anx7625 bridge driver must set DSC
> > > > > > en flag in .mode_set().
> > > > > > >
> > > > > > > >
> > > > > > > > b). Driver cannot get correctly dsc_en value from new bridge
> > > > > > > > state in
> > > > > > > > .atomic_pre_enable() or .atomic_enable().
> > > > > > >
> > > > > > > I don't get this. Why can it not?
> > > > > > I dumped the bridge state's point
> in .atomic_check(), .atomic_pre_enable()
> > > > > and .atomic_enable() as following:
> > > > > > at atomic_duplicate_state() allocated drm_bridge_state
> > > > > (0x0000000064d3ea49)
> > > > > > at atomic_check() drm_bridge_state (0x0000000064d3ea49)
> > > > > > at atomic_duplicate_state() allocated
> > > > > drm_bridge_state(0x00000000e0d6843d)
> > > > > > at atomic_check() drm_bridge_state (0x00000000e0d6843d)
> > > > > >
> > > > > > at atomic_pre_enable() drm_bridge_state (0x0000000081a4233a)
> > > > > > at atomic_enable() drm_bridge_state (0x0000000081a4233a) It
> seems
> > > > > > atomic_pre_enable() and atomic_enable() used different
> drm_bridge_state
> > > > > data structure.
> > > > >
> > > > > This should not be happening. Please check the dmesg with
> drm.debug=0x16.
> > > It
> > > > > might give you a clue.
> > > >
> > > > Hi Dmitry, the below is summarized kernel log, it is based on kernel v6.6.
> > >
> > > This is a very old kernel. Please don't use it for development. We are
> > > on the way to 6.14, development targets 6.15. Always use linux-next,
> > > drm-next, drm-misc-next or a similar branch. Using 6.6 means that the
> > > patch is completely untested on the contemporary kernels.
> > >
> >
> > Hi Dmitry, I failed to upgrade kernel to the latest 6.15 on our Chromium board.
>
> Maybe it's because there is no 6.15?
Sorry, I mean 6.14, I tried to porting code to 6.14, but failed.
>
> > So I cannot verify this patch on 6.15.
> > As I cannot verify on 6.15, and duplicate bridge state feature not working on
> v6.6,
> > and dsc_en flag is not necessary, I removed it in next version.
>
> I can't understand this comment. Please don't submit code that wasn't
> validated against recent enough kernels. v6.6 was published more than
> two years ago.
OK, so I remove dsc_en flags in next patch, this flag is no need anymore,
Driver always check currently pixel clock to decide whether enable DSC or not.
>
> >
> > >
> > > >
> > > > 1. MTK's DSC patch not upstream yet, so I cannot give a link, as log show,
> MTK
> > > call crtc atomic_enable
> > > > and will print out message "[drm:mtk_crtc_atomic_enable]
> > > mtk_crtc_atomic_enable 119", in this function,
> > > > MTK probe DSC flag and enable/disable encoder's DSC feature. And it
> called
> > > before
> > > > calling anx7625 bridge .atomic_pre_enable(). So it is too late if we set DSC
> > > parameters in .atomic_enable().
> > >
> > > What is 'probe DSC flag'? You should calculate your params
> in .atomic_check().
> >
> > I don't need to calculate it in .atomic_check(), check it in .mode_set()
>
> No. You must reject the mode if it can not be handled. Not being able to
> use DSC because the DSI host can't provide you with the DSC data is one
> of the examples.
I don't know whether the DSI host support DSC feature, and
I confirmed with MTK SOC MIPI develop engineer, there is no flag to tell bridge
driver that host support DSC or not.
So, I'll force set DSC parameters if pixel clock over 250M.
>
> >
> > >
> > > >
> > > > 2. With drm.debug=0x16, based on the below message, the
> drm_bridge_state
> > > in .atomic_enable() is
> > > > "0x00000000f555285e", I cannot find out where allocated it from kernel
> > > message,
> > >
> > > Which state is returned by drm_atomic_helper_bridge_reset()?
> > Removed dsc_en flag, no need to keep this flag any more.
> >
> > >
> > > > seems it is not allocated by .atomic_duplicate_state(), but driver will
> > > call .atomic_destroy_state() to destroy it.
> > > > Currently, we verified this patch on kernel v6.6, I'm not sure whether it is a
> bug
> > > on this kernel version.
> > > >
> > > > [ 1441.867169] anx7625: at atomic_duplicate_state() allocated
> > > drm_bridge_state (000000009c41f83d)
> > > > [ 1441.867173] mediatek-drm mediatek-drm.19.auto:
> > > [drm:drm_atomic_get_private_obj_state] Added new private object
> > > 00000000b3365a4b state 000000009c41f83d to 000000008a2c4e47
> > > > [ 1441.867194] anx7625: at atomic_check() drm_bridge_state
> > > (000000009c41f83d), drm_crtc_state(00000000d7a34c7e)
> > > > [ 1441.867204] anx7625: at atomic_destory_state() destroy
> > > state(0x000000009c41f83d)
> > > > [ 1441.875754] anx7625: at atomic_duplicate_state() allocated
> > > drm_bridge_state (000000004d0ab8c9)
> > > > [ 1441.875758] mediatek-drm mediatek-drm.19.auto:
> > > [drm:drm_atomic_get_private_obj_state] Added new private object
> > > 00000000b3365a4b state 000000004d0ab8c9 to 000000005bab791c
> > > > [ 1441.875783] anx7625: at atomic_check() drm_bridge_state
> > > (000000004d0ab8c9), drm_crtc_state(00000000fefeb2dd)
> > > > [ 1441.875805] anx7625 4-0058: [drm:anx7625_bridge_mode_set] drm
> mode
> > > set
> > > > [ 1441.875839] [drm:mtk_crtc_atomic_enable] mtk_crtc_atomic_enable
> 119
> > > > [ 1441.876649] mediatek-drm mediatek-drm.19.auto:
> > > [drm:drm_atomic_helper_commit_modeset_enables] enabling
> > > [ENCODER:41:DSI-41]
> > > > [ 1441.876655] anx7625: at atomic_pre_enable() drm_bridge_state
> > > (00000000f555285e)
> > > > [ 1441.879778] anx7625: at atomic_enable() drm_bridge_state
> > > (00000000f555285e)
> > > > [ 1441.886301] anx7625 4-0058: [drm:anx7625_dp_start] config dsi.
> > > > [ 1441.894841] anx7625 4-0058: [drm:anx7625_dp_start] compute
> > > M(14820608), N(691200), divider(2).
> > > > [ 1441.901921] anx7625 4-0058: [drm:anx7625_dp_start] success to config
> > > DSI
> > > > [ 1441.913360] mediatek-drm mediatek-drm.19.auto:
> > > [drm:drm_atomic_state_default_clear] Clearing atomic state
> > > 000000005bab791c
> > > > [ 1441.913383] anx7625: at atomic_destory_state() destroy
> > > state(0x00000000f555285e)
> > >
> > > This looks strange a bit. The driver has programmed the state in
> > > atomic_enable() and then it gets destroyed without .atomic_disable().
>
> --
> With best wishes
> Dmitry
Powered by blists - more mailing lists