Message-ID: <CAA8EJpoQgcG97Bq6EpTxWFzfOmZw8gaKm2q7ty1j++6NOSUOcQ@mail.gmail.com>
Date:   Mon, 27 Feb 2023 14:33:02 +0200
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     Abhinav Kumar <quic_abhinavk@...cinc.com>
Cc:     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, quic_sbillaka@...cinc.com,
        marijn.suijten@...ainline.org, freedreno@...ts.freedesktop.org,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 1/2] drm/msm/dpu: add dsc helper functions

On Mon, 27 Feb 2023 at 02:15, Abhinav Kumar <quic_abhinavk@...cinc.com> wrote:
>
>
>
> On 2/26/2023 5:13 AM, Dmitry Baryshkov wrote:
> > On 26/02/2023 02:16, Abhinav Kumar wrote:
> >> Hi Dmitry
> >>
> >> On 2/24/2023 3:57 PM, Dmitry Baryshkov wrote:
> >>> On Sat, 25 Feb 2023 at 01:51, Kuogee Hsieh <quic_khsieh@...cinc.com>
> >>> wrote:
> >>>>
> >>>>
> >>>> On 2/24/2023 1:13 PM, Dmitry Baryshkov wrote:
> >>>>> On 24/02/2023 21:40, Kuogee Hsieh wrote:
> >>>>>> Add DSC helper functions based on DSC configuration profiles to
> >>>>>> produce
> >>>>>> DSC related runtime parameters through both table look up and runtime
> >>>>>> calculation to support DSC on DPU.
> >>>>>>
> >>>>>> There are 6 different DSC configuration profiles are supported
> >>>>>> currently.
> >>>>>> DSC configuration profiles are differiented by 5 keys, DSC version
> >>>>>> (V1.1),
> >>>>>> chroma (444/422/420), colorspace (RGB/YUV), bpc(8/10),
> >>>>>> bpp (6/7/7.5/8/9/10/12/15) and SCR (0/1).
> >>>>>>
> >>>>>> Only DSC version V1.1 added and V1.2 will be added later.
> >>>>>
> >>>>> These helpers should go to drivers/gpu/drm/display/drm_dsc_helper.c
> >>>>> Also please check that they can be used for i915 or for amdgpu
> >>>>> (ideally for both of them).
> >>>>>
> >>>>> I didn't check the tables against the standard (or against the current
> >>>>> source code), will do that later.
> >>>>>
> >>>>>>
> >>>>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@...cinc.com>
> >>>>>> ---
> >>>>>>    drivers/gpu/drm/msm/Makefile                   |   1 +
> >>>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_dsc_helper.c | 209
> >>>>>> +++++++++++++++++++++++++
> >>>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_dsc_helper.h |  34 ++++
> >>>>>>    3 files changed, 244 insertions(+)
> >>>>>>    create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_dsc_helper.c
> >>>>>>    create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_dsc_helper.h
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/msm/Makefile
> >>>>>> b/drivers/gpu/drm/msm/Makefile
> >>>>>> index 7274c412..28cf52b 100644
> >>>>>> --- a/drivers/gpu/drm/msm/Makefile
> >>>>>> +++ b/drivers/gpu/drm/msm/Makefile
> >>>>>> @@ -65,6 +65,7 @@ msm-$(CONFIG_DRM_MSM_DPU) += \
> >>>>>>        disp/dpu1/dpu_hw_catalog.o \
> >>>>>>        disp/dpu1/dpu_hw_ctl.o \
> >>>>>>        disp/dpu1/dpu_hw_dsc.o \
> >>>>>> +    disp/dpu1/dpu_dsc_helper.o \
> >>>>>>        disp/dpu1/dpu_hw_interrupts.o \
> >>>>>>        disp/dpu1/dpu_hw_intf.o \
> >>>>>>        disp/dpu1/dpu_hw_lm.o \
> >>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_dsc_helper.c
> >>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_dsc_helper.c
> >>>>>> new file mode 100644
> >>>>>> index 00000000..88207e9
> >>>>>> --- /dev/null
> >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_dsc_helper.c
> >>>>>> @@ -0,0 +1,209 @@
> >>>>>> +// SPDX-License-Identifier: GPL-2.0-only
> >>>>>> +/*
> >>>>>> + * Copyright (c) 2023. Qualcomm Innovation Center, Inc. All rights
> >>>>>> reserved
> >>>>>> + */
> >>>>>> +
> >>>>>> +#include <drm/display/drm_dsc_helper.h>
> >>>>>> +#include "msm_drv.h"
> >>>>>> +#include "dpu_kms.h"
> >>>>>> +#include "dpu_hw_dsc.h"
> >>>>>> +#include "dpu_dsc_helper.h"
> >>>>>> +
> >>>>>> +
> >>>>>
> >>>>> Extra empty line
> >>>>>
> >>>>>> +#define DPU_DSC_PPS_SIZE       128
> >>>>>> +
> >>>>>> +enum dpu_dsc_ratio_type {
> >>>>>> +    DSC_V11_8BPC_8BPP,
> >>>>>> +    DSC_V11_10BPC_8BPP,
> >>>>>> +    DSC_V11_10BPC_10BPP,
> >>>>>> +    DSC_V11_SCR1_8BPC_8BPP,
> >>>>>> +    DSC_V11_SCR1_10BPC_8BPP,
> >>>>>> +    DSC_V11_SCR1_10BPC_10BPP,
> >>>>>> +    DSC_RATIO_TYPE_MAX
> >>>>>> +};
> >>>>>> +
> >>>>>> +
> >>>>>> +static u16 dpu_dsc_rc_buf_thresh[DSC_NUM_BUF_RANGES - 1] = {
> >>>>>> +        0x0e, 0x1c, 0x2a, 0x38, 0x46, 0x54,
> >>>>>> +        0x62, 0x69, 0x70, 0x77, 0x79, 0x7b, 0x7d, 0x7e
> >>>>>
> >>>>> Weird indentation
> >>>>>
> >>>>>> +};
> >>>>>> +
> >>>>>> +/*
> >>>>>> + * Rate control - Min QP values for each ratio type in
> >>>>>> dpu_dsc_ratio_type
> >>>>>> + */
> >>>>>> +static char
> >>>>>> dpu_dsc_rc_range_min_qp[DSC_RATIO_TYPE_MAX][DSC_NUM_BUF_RANGES] = {
> >>>>>> +    /* DSC v1.1 */
> >>>>>> +    {0, 0, 1, 1, 3, 3, 3, 3, 3, 3, 5, 5, 5, 7, 13},
> >>>>>> +    {0, 4, 5, 5, 7, 7, 7, 7, 7, 7, 9, 9, 9, 11, 17},
> >>>>>> +    {0, 4, 5, 6, 7, 7, 7, 7, 7, 7, 9, 9, 9, 11, 15},
> >>>>>> +    /* DSC v1.1 SCR and DSC v1.2 RGB 444 */
> >>>>>
> >>>>> What is SCR? Is there any reason to use older min/max Qp params
> >>>>> instead of always using the ones from the VESA-DSC-1.1 standard?
> >>>>
> >>>> Standards change request, some vendors may use scr to work with
> >>>> their panel.
> >>>>
> >>>> These table value are provided by system team.
> >>>
> >>> So, what will happen if we use values from 1.2 standard (aka 1.1 SCR
> >>> 1) with the older panel?
> >>>
> >>
> >> Standards change request means fixing errors/errata for the given
> >> standard. Those are typically released as a different spec.
> >>
> >> So I referred the DSC 1.1 SCR spec, and it does have a few differences
> >> in the table compared to DSC 1.1 which will get into DSC 1.2.
> >>
> >> Hence the table entries are same between DSC 1.1 SCR and DSC 1.2
> >>
> >> You are right, ideally DSC 1.2 should be backwards compatible with DSC
> >> 1.1 in terms of the values (thats what the spec says too) but I am not
> >> sure if we can expect every panel/DP monitor to be forward compatible
> >> without any SW change because it might need some firmware update (for
> >> the panel) or SW update to support that especially during transitions
> >> of the spec revisions (SCR to be precise).
> >>
> >> Typically we do below for DP monitors exactly for the same reason:
> >>
> >> DSC_ver_to_use = min(what_we_support, what_DP_monitor_supports) and
> >> use that table.
> >>
> >> For DSI panels, typically in the panel spec it should say whether the
> >> SCR version needs to be used because we have seen that for some panels
> >> ( I dont remember exactly which one ) based on which panel and which
> >> revision of the panel, it might not.
> >
> > So, what happens if we use DSC 1.1 SCR (= DSC 1.2) values with older
> > panel? Does it result in the broken image?
> >
> > I'm asking here, because I think that these parameters tune the
> > _encoder_. The decoder should be able to handle different compressed
> > streams as long as values fit into the required 'profile'.
> >
> Yes, this can cause screen corruption issues.
>
> The RC parameters table is used both in the encoder and in the PPS too
> and will be used to decode too.
>
> If we use the DSC 1.2 tables for a monitor/panel which advertizes that
> it supports only 1.1, we cannot be certain it will work.

But aren't those tables the recommended ones? So the panels should
work with a broader range of possible settings, shouldn't they?

Anyway, probably you have observed some kind of image corruption when
using DSC 1.1 SCR parameters with older panels.

>
>
> >>
> >> Thats why downstream started adding qcom,mdss-dsc-scr-version to the
> >> devicetree.
> >>
> >>>>>> +    {0, 0, 1, 1, 3, 3, 3, 3, 3, 3, 5, 5, 5, 9, 12},
> >>>>>> +    {0, 4, 5, 5, 7, 7, 7, 7, 7, 7, 9, 9, 9, 13, 16},
> >>>>>> +    {0, 4, 5, 6, 7, 7, 7, 7, 7, 7, 9, 9, 9, 11, 15},
> >>>
> >>>
> >



-- 
With best wishes
Dmitry

Powered by blists - more mailing lists