Re: [RFC PATCH 1/4] drm/msm/mdss: convert UBWC setup to use match data

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux