[RFC PATCH 1/4] drm/msm/mdss: convert UBWC setup to use match data
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Wed Jan 11 15:11:03 UTC 2023
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.
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.
>
> 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? 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.
>
> 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.
>
> - Marijn
>
>> That way we get rid of all register reads in this path which have
>> continuously bugged us with crashes.
>
> <snip>
--
With best wishes
Dmitry
More information about the dri-devel
mailing list