Re: [PATCH v1 12/14] drm/msm/disp/dpu1: revise timing engine programming to work for DSC

From: Kuogee Hsieh <quic_khsieh@quicinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	<dri-devel@lists.freedesktop.org>, <robdclark@gmail.com>,
	<sean@poorly.run>, <swboyd@chromium.org>, <dianders@chromium.org>,
	<vkoul@kernel.org>, <daniel@ffwll.ch>, <airlied@gmail.com>,
	<agross@kernel.org>, <andersson@kernel.org>
Cc: <quic_abhinavk@quicinc.com>, <quic_sbillaka@quicinc.com>,
	<freedreno@lists.freedesktop.org>,
	<linux-arm-msm@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v1 12/14] drm/msm/disp/dpu1: revise timing engine programming to work for DSC
Date: Tue, 24 Jan 2023 09:55:24 -0800	[thread overview]
Message-ID: <3588a5d0-ca28-918f-e072-35f15a5a5132@quicinc.com> (raw)
In-Reply-To: <8392e1f3-8459-4408-41de-564a41980b4c@linaro.org>

On 1/24/2023 1:11 AM, Dmitry Baryshkov wrote:
> 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@quicinc.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.

This timing engine code is derived from our downstream code directly and 
it has been used at many mobile devices by many vendors for many years 
already.

On the other words, it had been tested very thorough and works on 
dsi/dp/hdmi/dsc/widebus applications.

When i brought dsc v1.2 over, I just merged it over and did not consider 
too much.

Can we adapt this code so that both upstream and down stream shared same 
timing engine programming so that easier to maintain?

>
>>         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,
>