On 2023-01-11 17:11:03, Dmitry Baryshkov wrote:
> On 11/01/2023 10:44, Marijn Suijten wrote:
> > On 2023-01-09 12:32:18, Abhinav Kumar wrote:
> > <snip>
> >>>> On 12/7/2022 4:08 PM, Dmitry Baryshkov wrote:
> > <snip>
> >>>>> +struct msm_mdss_data {
> >>>>> + u32 ubwc_version;
> >>>>> + u32 ubwc_swizzle;
> >>>>> + u32 ubwc_static;
> >>>>> + u32 highest_bank_bit;
> >>>>> + u32 macrotile_mode;
> >>>>> +};
> >
> > This magic struct could really use some documentation, otherwise users
> > will have no idea what fields to set (or omit) nor what values to use.
> > For example decoder 2.0 seems to only use ubwc_static as a sort of magic
> > "we don't know what the bits in UBWC_STATIC mean", whereas decoder 3.0
> > reconstructs this field entirely from the other parameters. Decoder 4.0
> > however does the same, but _also_ embeds this uwbc_static magic value
> > back into the register value....?
>
> On the bright side these magic values correspond 1:1 to the vendor dtsi
> and to the part of DPU hw catalog. It would be nice to know the bit used
> by decoder 2.0, but I fear that we'd have to resort to wild guesses
> unless Qualcomm decides to disclose that information.
If downstream has these fields verbatim that makes it easier to
copy-paste, agreed.
> As for dec 4.0 and ubwc_static. I fear that it's just somebody (writing
> downstream DT parsing) reused the ubwc-static name for the bitfield
> which in reality has some sensible name.
Yes, ugh.
> > Also read on below about checking "compatibility" between this struct
> > and the decoder version, and why I feel this struct (versus mandatory
> > function arguments) makes this struct less robust.
> >
> >>>>> struct msm_mdss {
> >>>>> struct device *dev;
> >>>>> @@ -40,6 +48,7 @@ struct msm_mdss {
> >>>>> unsigned long enabled_mask;
> >>>>> struct irq_domain *domain;
> >>>>> } irq_controller;
> >>>>> + const struct msm_mdss_data *mdss_data;
> >>>>> struct icc_path *path[2];
> >>>>> u32 num_paths;
> >>>>> };
> >>>>> @@ -180,46 +189,40 @@ static int _msm_mdss_irq_domain_add(struct
> >>>>> msm_mdss *msm_mdss)
> >>>>> #define UBWC_3_0 0x30000000
> >>>>> #define UBWC_4_0 0x40000000
> >>>>> -static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss,
> >>>>> - u32 ubwc_static)
> >>>>> +static void msm_mdss_setup_ubwc_dec_20(struct msm_mdss *msm_mdss)
> >>>>> {
> >>>>> - writel_relaxed(ubwc_static, msm_mdss->mmio + UBWC_STATIC);
> >>>>> + const struct msm_mdss_data *data = msm_mdss->mdss_data;
> >>>>> +
> >>>>> + writel_relaxed(data->ubwc_static, msm_mdss->mmio + UBWC_STATIC);
> >>>>> }
> >>>>> -static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss,
> >>>>> - unsigned int ubwc_version,
> >>>>> - u32 ubwc_swizzle,
> >>>>> - u32 highest_bank_bit,
> >>>>> - u32 macrotile_mode)
> >>>>> +static void msm_mdss_setup_ubwc_dec_30(struct msm_mdss *msm_mdss)
> >>>>> {
> >>>>> - u32 value = (ubwc_swizzle & 0x1) |
> >>>>> - (highest_bank_bit & 0x3) << 4 |
> >>>>> - (macrotile_mode & 0x1) << 12;
> >>>>> + const struct msm_mdss_data *data = msm_mdss->mdss_data;
> >>>>> + u32 value = (data->ubwc_swizzle & 0x1) |
> >>>>> + (data->highest_bank_bit & 0x3) << 4 |
> >>>>> + (data->macrotile_mode & 0x1) << 12;
> >>>>> - if (ubwc_version == UBWC_3_0)
> >>>>> + if (data->ubwc_version == UBWC_3_0)
> >>>>> value |= BIT(10);
> >>>>> - if (ubwc_version == UBWC_1_0)
> >>>>> + if (data->ubwc_version == UBWC_1_0)
> >>>>> value |= BIT(8);
> >>>>> writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
> >>>>> }
> >>>>> -static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss,
> >>>>> - unsigned int ubwc_version,
> >>>>> - u32 ubwc_swizzle,
> >>>>> - u32 ubwc_static,
> >>>>> - u32 highest_bank_bit,
> >>>>> - u32 macrotile_mode)
> >>>>> +static void msm_mdss_setup_ubwc_dec_40(struct msm_mdss *msm_mdss)
> >>>>> {
> >>>>> - u32 value = (ubwc_swizzle & 0x7) |
> >>>>> - (ubwc_static & 0x1) << 3 |
> >>>>> - (highest_bank_bit & 0x7) << 4 |
> >>>>> - (macrotile_mode & 0x1) << 12;
> >>>>> + const struct msm_mdss_data *data = msm_mdss->mdss_data;
> >>>>> + u32 value = (data->ubwc_swizzle & 0x7) |
> >>>>> + (data->ubwc_static & 0x1) << 3 |
> >>>>> + (data->highest_bank_bit & 0x7) << 4 |
> >>>>> + (data->macrotile_mode & 0x1) << 12;
> >>>>> writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
> >>>>> - if (ubwc_version == UBWC_3_0) {
> >>>>> + if (data->ubwc_version == UBWC_3_0) {
> >>>>> writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
> >>>>> writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
> >>>>> } else {
> >>>>> @@ -232,6 +235,7 @@ static int msm_mdss_enable(struct msm_mdss
> >>>>> *msm_mdss)
> >>>>> {
> >>>>> int ret;
> >>>>> u32 hw_rev;
> >>>>> + u32 ubwc_dec_hw_version;
> >>>>> /*
> >>>>> * Several components have AXI clocks that can only be turned
> >>>>> on if
> >>>>> @@ -250,53 +254,36 @@ static int msm_mdss_enable(struct msm_mdss
> >>>>> *msm_mdss)
> >>>>> * HW_REV requires MDSS_MDP_CLK, which is not enabled by the
> >>>>> mdss on
> >>>>> * mdp5 hardware. Skip reading it for now.
> >>>>> */
> >>>>> - if (msm_mdss->is_mdp5)
> >>>>> + if (msm_mdss->is_mdp5 || !msm_mdss->mdss_data)
> >>>>> return 0;
> >>>>> hw_rev = readl_relaxed(msm_mdss->mmio + HW_REV);
> >>
> >> hw_rev is not used anymore now so why not just drop that reg read
> >> altogether.
> >>
> >>>>> dev_dbg(msm_mdss->dev, "HW_REV: 0x%x\n", hw_rev);
> >>>>> +
> >>>>> + ubwc_dec_hw_version = readl_relaxed(msm_mdss->mmio +
> >>>>> UBWC_DEC_HW_VERSION);
> >>
> >> If we are going to tie UBWC version to the HW compatible match, then
> >> even this register read can be skipped and instead you can add
> >> ubwc_dec_hw_version to your match data struct and skip this read as well.
> >
> > I have suggested in IRC to keep this register read, and utilize it to at
> > least sanity check the configuration. You are right that the DPU HW
> > version already describes what UWBC decoder version is used, but we're
> > are already questioning whether it was ported correctly for SM6115. A
> > WARN() that catches a mismatch between what was written in the "catalog"
> > (or this match table) versus what the hardware reports would have gone a
> > long way.
>
> Well... Sanity checking here means we do not trust the kernel. And whom
> we can trust then?
I have no reason to trust the kernel here. Knowing the read-back value
might even be necessary to decipher the "downstream" kernel, since it
doesn't specify the decoder /hardware/ version but only the data format?
> Note, that for 6115 I had a question regarding the
> ubwc_version stated in the comment, not in the code. I asked for
> UBWC_DEC_HW_VERSION value just to be sure.
Right, that is some weird paraphrasing. Is it the UBWC_2_0 data format
(because there's nothing in dec_20 performing a conditional based on
this) and only coincidentally being the same as the HW decoder version?
> > This is especially relevant with the new struct where fields are
> > (un)used depending on the UBWC HW decoder version, making for an extra
> > exercise to the developer to double-check whether their struct values
> > are taken into account or not (or if used ones are accidentally
> > omitted).
>
> Granted the overlay between DPU catalog and MDSS device data maybe we
> should make DPU ask MDSS for the ubwc settings.
Is DPU also holding on to these values? I was more so referring to the
fact that the HW_DEC version determines what specific fields are read
and what are not, which may not be immediately obvious when adding a
struct instance unless reading the code.
- Marijn
