| 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: <8392e1f3-8459-4408-41de-564a41980b4c@linaro.org>
Date: Tue, 24 Jan 2023 11:11:25 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Kuogee Hsieh <quic_khsieh@...cinc.com>,
dri-devel@...ts.freedesktop.org, robdclark@...il.com,
sean@...rly.run, swboyd@...omium.org, dianders@...omium.org,
vkoul@...nel.org, daniel@...ll.ch, airlied@...il.com,
agross@...nel.org, andersson@...nel.org
Cc: quic_abhinavk@...cinc.com, quic_sbillaka@...cinc.com,
freedreno@...ts.freedesktop.org, linux-arm-msm@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 12/14] drm/msm/disp/dpu1: revise timing engine
programming to work for DSC
On 23/01/2023 20:24, Kuogee Hsieh wrote:
> Current implementation timing engine programming does not consider
> compression factors. This patch add consideration of DSC factors
> while programming timing engine.
>
> Signed-off-by: Kuogee Hsieh <quic_khsieh@...cinc.com>
> ---
> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 2 +
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 14 ++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 132 +++++++++++++--------
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 10 +-
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h | 6 +-
> 5 files changed, 110 insertions(+), 54 deletions(-)
>
[skipped]
> @@ -113,82 +124,96 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
> /* read interface_cfg */
> intf_cfg = DPU_REG_READ(c, INTF_CONFIG);
>
> - if (ctx->cap->type == INTF_DP)
> + if (ctx->cap->type == INTF_EDP || ctx->cap->type == INTF_DP)
> dp_intf = true;
>
> hsync_period = p->hsync_pulse_width + p->h_back_porch + p->width +
> - p->h_front_porch;
> + p->h_front_porch;
> vsync_period = p->vsync_pulse_width + p->v_back_porch + p->height +
> - p->v_front_porch;
> + p->v_front_porch;
Actually I went on through the history and found the previous
submission, https://patchwork.freedesktop.org/patch/471505/.
Exactly the same piece of code. Did you expect that the comments will be
different this time?
I really hoped that at that time we already went through this. But it
seems I was wrong. That series went through v10 or v12 before being
accepted. And it was just adding wide_bus_en. Back at that time we
lightly discussed that the code will receive compression support. But I
never expected to see the original submission again.
It might sound bad, but could you please find somebody who can do
internal review for you? Good internal review.
That said, I really do not expect to see v2 before the whole series is
reworked, restructured and prepared for the review on your side.
>
> display_v_start = ((p->vsync_pulse_width + p->v_back_porch) *
> - hsync_period) + p->hsync_skew;
> + hsync_period) + p->hsync_skew;
> display_v_end = ((vsync_period - p->v_front_porch) * hsync_period) +
> - p->hsync_skew - 1;
> + p->hsync_skew - 1;
> +
> + hsync_ctl = (hsync_period << 16) | p->hsync_pulse_width;
>
> hsync_start_x = p->h_back_porch + p->hsync_pulse_width;
> hsync_end_x = hsync_period - p->h_front_porch - 1;
>
> - if (p->width != p->xres) { /* border fill added */
> - active_h_start = hsync_start_x;
> - active_h_end = active_h_start + p->xres - 1;
> - } else {
> - active_h_start = 0;
> - active_h_end = 0;
> - }
> -
> - if (p->height != p->yres) { /* border fill added */
> - active_v_start = display_v_start;
> - active_v_end = active_v_start + (p->yres * hsync_period) - 1;
> - } else {
> - active_v_start = 0;
> - active_v_end = 0;
> - }
> -
> - if (active_h_end) {
> - active_hctl = (active_h_end << 16) | active_h_start;
> - intf_cfg |= INTF_CFG_ACTIVE_H_EN;
> - } else {
> - active_hctl = 0;
> - }
> -
> - if (active_v_end)
> - intf_cfg |= INTF_CFG_ACTIVE_V_EN;
> -
> - hsync_ctl = (hsync_period << 16) | p->hsync_pulse_width;
> - display_hctl = (hsync_end_x << 16) | hsync_start_x;
> -
> /*
> * DATA_HCTL_EN controls data timing which can be different from
> * video timing. It is recommended to enable it for all cases, except
> * if compression is enabled in 1 pixel per clock mode
> */
> + if (!p->compression_en || p->wide_bus_en)
> + intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN;
> +
> if (p->wide_bus_en)
> - intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN;
> + intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
>
> + /*
> + * If widebus is disabled:
> + * For uncompressed stream, the data is valid for the entire active
> + * window period.
> + * For compressed stream, data is valid for a shorter time period
> + * inside the active window depending on the compression ratio.
> + *
> + * If widebus is enabled:
> + * For uncompressed stream, data is valid for only half the active
> + * window, since the data rate is doubled in this mode.
> + * p->width holds the adjusted width for DP but unadjusted width for DSI
> + * For compressed stream, data validity window needs to be adjusted for
> + * compression ratio and then further halved.
> + */
> data_width = p->width;
>
> + if (p->compression_en) {
> + if (p->wide_bus_en)
> + data_width = DIV_ROUND_UP(p->dce_bytes_per_line, 6);
> + else
> + data_width = DIV_ROUND_UP(p->dce_bytes_per_line, 3);
> + } else if (!dp_intf && p->wide_bus_en) {
> + data_width = p->width >> 1;
> + } else {
> + data_width = p->width;
> + }
> +
> hsync_data_start_x = hsync_start_x;
> hsync_data_end_x = hsync_start_x + data_width - 1;
>
> + display_hctl = (hsync_end_x << 16) | hsync_start_x;
> display_data_hctl = (hsync_data_end_x << 16) | hsync_data_start_x;
>
> if (dp_intf) {
> /* DP timing adjustment */
> display_v_start += p->hsync_pulse_width + p->h_back_porch;
> display_v_end -= p->h_front_porch;
> + }
> +
> + intf_cfg |= INTF_CFG_ACTIVE_H_EN;
> + intf_cfg |= INTF_CFG_ACTIVE_V_EN;
> + active_h_start = hsync_start_x;
> + active_h_end = active_h_start + p->xres - 1;
> + active_v_start = display_v_start;
> + active_v_end = active_v_start + (p->yres * hsync_period) - 1;
>
> - active_h_start = hsync_start_x;
> - active_h_end = active_h_start + p->xres - 1;
> - active_v_start = display_v_start;
> - active_v_end = active_v_start + (p->yres * hsync_period) - 1;
> + active_hctl = (active_h_end << 16) | active_h_start;
>
> - active_hctl = (active_h_end << 16) | active_h_start;
> + if (dp_intf) {
> display_hctl = active_hctl;
>
> - intf_cfg |= INTF_CFG_ACTIVE_H_EN | INTF_CFG_ACTIVE_V_EN;
> + if (p->compression_en) {
> + active_data_hctl = (hsync_start_x + p->extra_dto_cycles) << 16;
> + active_data_hctl += hsync_start_x;
> +
> + display_data_hctl = active_data_hctl;
> + }
> }
>
> + _check_and_set_comp_bit(ctx, p->dsc_4hs_merge, p->compression_en, &intf_cfg2);
> +
> den_polarity = 0;
> if (ctx->cap->type == INTF_HDMI) {
> hsync_polarity = p->yres >= 720 ? 0 : 1;
> @@ -202,7 +227,7 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
--
With best wishes
Dmitry
Powered by blists - more mailing lists