[Freedreno] [PATCH] drm/msm: less magic numbers in msm_mdss_enable
Abhinav Kumar
quic_abhinavk at quicinc.com
Wed Jun 8 22:35:57 UTC 2022
On 6/8/2022 3:30 PM, Dmitry Baryshkov wrote:
> On Wed, 8 Jun 2022 at 22:29, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>
>>
>>
>> On 6/2/2022 1:13 PM, Dmitry Baryshkov wrote:
>>> On Thu, 2 Jun 2022 at 21:18, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 6/1/2022 1:04 PM, Dmitry Baryshkov wrote:
>>>>> On Wed, 1 Jun 2022 at 20:38, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 6/1/2022 2:46 AM, Dmitry Baryshkov wrote:
>>>>>>> On Wed, 1 Jun 2022 at 01:01, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>>>>>>> On 5/31/2022 5:18 AM, Dmitry Baryshkov wrote:
>>>>>>>>> Replace magic register writes in msm_mdss_enable() with version that
>>>>>>>>> contains less magic and more variable names that can be traced back to
>>>>>>>>> the dpu_hw_catalog or the downstream dtsi files.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>>>>>>>> ---
>>>>>>>>> drivers/gpu/drm/msm/msm_mdss.c | 79 ++++++++++++++++++++++++++++++----
>>>>>>>>> 1 file changed, 71 insertions(+), 8 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>> index 0454a571adf7..2a48263cd1b5 100644
>>>>>>>>> --- a/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>> +++ b/drivers/gpu/drm/msm/msm_mdss.c
>>>>>>>>> @@ -21,6 +21,7 @@
>>>>>>>>> #define HW_REV 0x0
>>>>>>>>> #define HW_INTR_STATUS 0x0010
>>>>>>>>>
>>>>>>>>> +#define UBWC_DEC_HW_VERSION 0x58
>>>>>>>>> #define UBWC_STATIC 0x144
>>>>>>>>> #define UBWC_CTRL_2 0x150
>>>>>>>>> #define UBWC_PREDICTION_MODE 0x154
>>>>>>>>> @@ -132,9 +133,63 @@ static int _msm_mdss_irq_domain_add(struct msm_mdss *msm_mdss)
>>>>>>>>> return 0;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> +#define UBWC_1_0 0x10000000
>>>>>>>>> +#define UBWC_2_0 0x20000000
>>>>>>>>> +#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)
>>>>>>>>> +{
>>>>>>>>> + writel_relaxed(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)
>>>>>>>>> +{
>>>>>>>>> + u32 value = (ubwc_swizzle & 0x1) |
>>>>>>>>> + (highest_bank_bit & 0x3) << 4 |
>>>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>>>> +
>>>>>>>>> + if (ubwc_version == UBWC_3_0)
>>>>>>>>> + value |= BIT(10);
>>>>>>>>> +
>>>>>>>>> + if (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)
>>>>>>>>> +{
>>>>>>>>> + u32 value = (ubwc_swizzle & 0x7) |
>>>>>>>>> + (ubwc_static & 0x1) << 3 |
>>>>>>>>> + (highest_bank_bit & 0x7) << 4 |
>>>>>>>>> + (macrotile_mode & 0x1) << 12;
>>>>>>>>> +
>>>>>>>>> + writel_relaxed(value, msm_mdss->mmio + UBWC_STATIC);
>>>>>>>>> +
>>>>>>>>> + if (ubwc_version == UBWC_3_0) {
>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>>>> + writel_relaxed(0, msm_mdss->mmio + UBWC_PREDICTION_MODE);
>>>>>>>>> + } else {
>>>>>>>>> + writel_relaxed(2, msm_mdss->mmio + UBWC_CTRL_2);
>>>>>>>>> + writel_relaxed(1, msm_mdss->mmio + UBWC_PREDICTION_MODE);
>>>>>>>>> + }
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>
>>>>>>>> Is it possible to unify the above functions by having the internal
>>>>>>>> ubwc_version checks?
>>>>>>>
>>>>>>> Note, it's not the ubwc_version, it is the ubwc_dec_hw_version. And
>>>>>>> also different functions take different sets of arguments.
>>>>>>>
>>>>>>>> It seems like msm_mdss_setup_ubwc_dec_xxx can keep growing.
>>>>>>>>
>>>>>>>> I have not looked into each bit programming but from the top level so
>>>>>>>> feel free to correct if wrong but it seems both do write UBWC_STATIC
>>>>>>>> (different values based on different UBWC versions) and write some extra
>>>>>>>> registers based on version
>>>>>>>
>>>>>>> This is what both the current code and the downstream do. See
>>>>>>> https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/zeus-s-oss/techpack/display-drivers/msm/sde/sde_hw_top.c#L312
>>>>>>>
>>>>>>
>>>>>> Thanks for pointing to the downstream method for this,
>>>>>>
>>>>>> This is exactly what i was also suggesting to do when I mentioned
>>>>>> unifying the above functions.
>>>>>>
>>>>>> So instead of having a separate function for each version why not handle
>>>>>> all the versions in the same function like what the link you have shown
>>>>>> does.
>>>>>
>>>>> I wouldn't like that. The downstream uses hw_catalog to pass all
>>>>> possible parameters. We do not, so we'd have a whole set of artificial
>>>>> values.
>>>>>
>>>>
>>>> Now that you brought that up, why cannot even upstream dpu start using
>>>> catalog for ubwc settings?
>>>
>>> Because msm_mdss lives out of disp/dpu1. And using the disp/dpu1 for
>>> it would be an inversion of dependencies.
>>> I like the fact that msm_mdss is independent of mdp/dpu drivers and I
>>> do not want to add such dependency.
>>>
>>
>> Ok, so I think this function itself is placed incorrectly. It should not
>> be in msm_mdss.c and should in the DPU folder.
>>
>> This check tells me that this will not be executed for mdp5 devices.
>>
>> /*
>> * 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)
>> return 0;
>
> This condition should be changed to check for the MDP_CLK being
> available in the clocks array rather than checking for is_mdp5. I'd
> like to phase is_mdp5 away at some point.
>
>> In that case, what prevents us from moving this to dpu and start using
>> catalog for this?
>
> Because there is nothing tying mdss and dpu drivers. For example, is
> the msm8998 (3.0.0) the DPU or MDP5 device? MSM8996?
> Neither struct msm_mdss nor the MDSS device itself are accessible
> through the msm_drv (or dpu_kms).
> I think trying to invent such a link would make the code worse.
>
Right, what I am trying to mention with that check is that means that
code does not run today for mdp5 and it still works fine.
So why not just move it to DPU first to carry less burden of these extra
register settings which are unused today for mdp5 anyway.
>>>> /* struct dpu_mdp_cfg : MDP TOP-BLK instance info
>>>> * @id: index identifying this block
>>>> * @base: register base offset to mdss
>>>> * @features bit mask identifying sub-blocks/features
>>>> * @highest_bank_bit: UBWC parameter
>>>> * @ubwc_static: ubwc static configuration
>>>> * @ubwc_swizzle: ubwc default swizzle setting
>>>> * @clk_ctrls clock control register definition
>>>> */
>>>> struct dpu_mdp_cfg {
>>>> DPU_HW_BLK_INFO;
>>>> u32 highest_bank_bit;
>>>> u32 ubwc_swizzle;
>>>> struct dpu_clk_ctrl_reg clk_ctrls[DPU_CLK_CTRL_MAX];
>>>> };
>>>>
>>>> We already do seem to have a couple of parameters. have to add the others.
>>>>
>>>> That way the number of functions wont keep growing.
>
>
More information about the Freedreno
mailing list