Re: [PATCH 4/6] drm/msm/mdss: populate missing data

[Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>]

On 7/26/2023 3:58 PM, Dmitry Baryshkov wrote:
> On Thu, 27 Jul 2023 at 01:30, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
> > On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote:
> > > As we are going to use MDSS data for DPU programming, populate missing
> > > MDSS data. The UBWC 1.0 and no UBWC cases do not require MDSS
> > > programming, so skip them.
> >
> > Can you pls point me to the downstream references you used for msm8998?
>
> msm-3.18, drivers/video/msm/mdss/mdss_mdp.c
>
> See the function mdss_mdp_hw_rev_caps_init(). It sets has_ubwc for MDP
> 1.07 (msm8996), 1.14 (msm8937) / 1.16  (msm8953) and 3.0 (msm8998).

It sets has_ubwc but I still cannot locate where it says version is 1.0.
Because the 0x58 register reads 0 and not 1 for msm8998.

> > Was that just taken from catalog? If so I would ask for the reference
> > for the catalog too.
> >
> > As per the register the decoder version is 0x0 and not 1.
> >
> > Even encoder version is 0 from what i see and not 1. Thats why a
> > dec_version of UBWC_1_0 is not doing anything i assume.
> >
> > Some additional questions:
> >
> > 1) Does the whole chunk in dpu_hw_sspp_setup_format() which handles ubwc
> > programming need to be protected by if (ctx->ubwc) now ?
>
> It's hard to discuss the question which is irrelevant for this patch.
> Nevertheless, yes, it needs to be protected because e.g. qcm2290
> doesn't have UBWC support.

Sorry but your commit text made me look into this function and wonder 
now what happens to that code. But I will continue this in this next 
patch so that you can see the code and the question together.

> > 2) The values of UBWC_x_x dont match the values of DPU_HW_UBWC_VER_xx.
> > What was the reason for the catalog to go with DPU_HW_UBWC_VER_xx in the
> > catalog for the encoder version in the first place? Because looking at
> > the registers UBWC_x_x is the correct value.
>
> Huh. This definitely should be asked next to the code that you wish to
> discuss. The DPU_HW_UBWC_VER_xx values come from the first DPU
> revision.

Sure, so again the next patch.

> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > ---
> > >    drivers/gpu/drm/msm/msm_mdss.c | 21 +++++++++++++++++++--
> > >    1 file changed, 19 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> > > index ed836c659688..9bb7be4b9ebb 100644
> > > --- a/drivers/gpu/drm/msm/msm_mdss.c
> > > +++ b/drivers/gpu/drm/msm/msm_mdss.c
> > > @@ -264,6 +264,10 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss)
> > >         * UBWC_n and the rest of params comes from hw data.
> > >         */
> > >        switch (msm_mdss->mdss_data->ubwc_dec_version) {
> > > +     case 0: /* no UBWC */
> > > +     case UBWC_1_0:
> > > +             /* do nothing */
> > > +             break;
> > >        case UBWC_2_0:
> > >                msm_mdss_setup_ubwc_dec_20(msm_mdss);
> > >                break;
> > > @@ -502,10 +506,22 @@ static int mdss_remove(struct platform_device *pdev)
> > >        return 0;
> > >    }
> > >
> > > +static const struct msm_mdss_data msm8998_data = {
> > > +     .ubwc_enc_version = UBWC_1_0,
> > > +     .ubwc_dec_version = UBWC_1_0,
> > > +     .highest_bank_bit = 1,
> > > +};
> > > +
> > > +static const struct msm_mdss_data qcm2290_data = {
> > > +     /* no UBWC */
> > > +     .highest_bank_bit = 0x2,
> > > +};
> > > +
> > >    static const struct msm_mdss_data sc7180_data = {
> > >        .ubwc_enc_version = UBWC_2_0,
> > >        .ubwc_dec_version = UBWC_2_0,
> > >        .ubwc_static = 0x1e,
> > > +     .highest_bank_bit = 0x3,
> > >    };
> > >
> > >    static const struct msm_mdss_data sc7280_data = {
> > > @@ -550,6 +566,7 @@ static const struct msm_mdss_data sm6115_data = {
> > >        .ubwc_dec_version = UBWC_2_0,
> > >        .ubwc_swizzle = 7,
> > >        .ubwc_static = 0x11f,
> > > +     .highest_bank_bit = 0x1,
> > >    };
> > >
> > >    static const struct msm_mdss_data sm8250_data = {
> > > @@ -574,8 +591,8 @@ static const struct msm_mdss_data sm8550_data = {
> > >
> > >    static const struct of_device_id mdss_dt_match[] = {
> > >        { .compatible = "qcom,mdss" },
> > > -     { .compatible = "qcom,msm8998-mdss" },
> > > -     { .compatible = "qcom,qcm2290-mdss" },
> > > +     { .compatible = "qcom,msm8998-mdss", .data = &msm8998_data },
> > > +     { .compatible = "qcom,qcm2290-mdss", .data = &qcm2290_data },
> > >        { .compatible = "qcom,sdm845-mdss", .data = &sdm845_data },
> > >        { .compatible = "qcom,sc7180-mdss", .data = &sc7180_data },
> > >        { .compatible = "qcom,sc7280-mdss", .data = &sc7280_data },