Received-SPF: pass (google.com: domain of linux-kernel+bounces-56997-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240106-fd-migrate-mdp5-v3-0-3d2750378063@linaro.org>
 <20240106-fd-migrate-mdp5-v3-1-3d2750378063@linaro.org> <89d02d5c-4af1-9f40-483f-1efb39b2a33d@quicinc.com>
In-Reply-To: <89d02d5c-4af1-9f40-483f-1efb39b2a33d@quicinc.com>
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date: Wed, 7 Feb 2024 21:59:28 +0200
Message-ID: <CAA8EJpro3jVPDAGWdpFiukCHTVif--Y-ZhEw=ir7U3ABsa_qbA@mail.gmail.com>
Subject: Re: [PATCH v3 1/4] drm/msm/mdss: generate MDSS data for MDP5 platforms
To: Abhinav Kumar <quic_abhinavk@quicinc.com>
Cc: Rob Clark <robdclark@gmail.com>, Sean Paul <sean@poorly.run>, 
	Marijn Suijten <marijn.suijten@somainline.org>, David Airlie <airlied@gmail.com>, 
	Daniel Vetter <daniel@ffwll.ch>, linux-arm-msm@vger.kernel.org, 
	freedreno@lists.freedesktop.org, linux-kernel@vger.kernel.org, 
	dri-devel@lists.freedesktop.org
Content-Type: text/plain; charset="UTF-8"

On Wed, 7 Feb 2024 at 20:25, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 1/5/2024 3:34 PM, Dmitry Baryshkov wrote:
> > Older (mdp5) platforms do not use per-SoC compatible strings. Instead
> > they use a single compat entry 'qcom,mdss'. To facilitate migrating
> > these platforms to the DPU driver provide a way to generate the MDSS /
> > UBWC data at runtime, when the DPU driver asks for it.
> >
> > It is not possible to generate this data structure at the probe time,
> > since some platforms might not have MDP_CLK enabled, which makes reading
> > HW_REV register return 0.
> >
>
> I would have expected a crash if clock was not enabled and we tried to
> access the hw_rev register.

No, for all the platforms I tested it returns 0 instead.
However this doesn't make any difference, we don't read the register
in MDP5 case until all clocks are enabled.

>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/msm/msm_mdss.c | 51 ++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 51 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> > index 455b2e3a0cdd..566a5dd5b8e8 100644
> > --- a/drivers/gpu/drm/msm/msm_mdss.c
> > +++ b/drivers/gpu/drm/msm/msm_mdss.c
> > @@ -3,6 +3,7 @@
> >    * Copyright (c) 2018, The Linux Foundation
> >    */
> >
> > +#include <linux/bitfield.h>
> >   #include <linux/clk.h>
> >   #include <linux/delay.h>
> >   #include <linux/interconnect.h>
> > @@ -213,6 +214,49 @@ static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss)
> >       }
> >   }
> >
> > +#define MDSS_HW_MAJ_MIN              GENMASK(31, 16)
> > +
> > +#define MDSS_HW_MSM8996              0x1007
> > +#define MDSS_HW_MSM8937              0x100e
> > +#define MDSS_HW_MSM8956              0x1010
>
> This should be 0x100B in the docs I see.

Yes, I mixed MSM8956 and MSM8953 here. The code support the latter one.

>
> > +#define MDSS_HW_MSM8998              0x3000
> > +#define MDSS_HW_SDM660               0x3002
> > +#define MDSS_HW_SDM630               0x3003
> > +
> > +/*
> > + * MDP5 platforms use generic qcom,mdp5 compat string, so we have to generate this data
> > + */
> > +static const struct msm_mdss_data *msm_mdss_generate_mdp5_mdss_data(struct msm_mdss *mdss)
> > +{
> > +     struct msm_mdss_data *data;
> > +     u32 hw_rev;
> > +
> > +     data = devm_kzalloc(mdss->dev, sizeof(*data), GFP_KERNEL);
> > +     if (!data)
> > +             return NULL;
> > +
> > +     hw_rev = readl_relaxed(mdss->mmio + HW_REV);
> > +     hw_rev = FIELD_GET(MDSS_HW_MAJ_MIN, hw_rev);
> > +
> > +     if (hw_rev == MDSS_HW_MSM8996 ||
> > +         hw_rev == MDSS_HW_MSM8937 ||
> > +         hw_rev == MDSS_HW_MSM8956 ||
> > +         hw_rev == MDSS_HW_MSM8998 ||
> > +         hw_rev == MDSS_HW_SDM660 ||
> > +         hw_rev == MDSS_HW_SDM630) {
> > +             data->ubwc_dec_version = UBWC_1_0;
> > +             data->ubwc_enc_version = UBWC_1_0;
> > +     }
> > +
> > +     if (hw_rev == MDSS_HW_MSM8996 ||
> > +         hw_rev == MDSS_HW_MSM8998)
> > +             data->highest_bank_bit = 2;
> > +     else
> > +             data->highest_bank_bit = 1;
> > +
> > +     return data;
> > +}
> > +
> >   const struct msm_mdss_data *msm_mdss_get_mdss_data(struct device *dev)
> >   {
> >       struct msm_mdss *mdss;
> > @@ -222,6 +266,13 @@ const struct msm_mdss_data *msm_mdss_get_mdss_data(struct device *dev)
> >
> >       mdss = dev_get_drvdata(dev);
> >
> > +     /*
> > +      * We could not do it at the probe time, since hw revision register was
> > +      * not readable. Fill data structure now for the MDP5 platforms.
> > +      */
> > +     if (!mdss->mdss_data && mdss->is_mdp5)
> > +             mdss->mdss_data = msm_mdss_generate_mdp5_mdss_data(mdss);
> > +
> >       return mdss->mdss_data;
> >   }
> >
> >


-- 
With best wishes
Dmitry