[Freedreno] [PATCH 2/3] drm/msm/dpu: Add MISR register support for interface
Jessica Zhang
quic_jesszhan at quicinc.com
Fri Jun 3 01:00:15 UTC 2022
On 6/2/2022 3:31 PM, Dmitry Baryshkov wrote:
> On 27/05/2022 23:11, Jessica Zhang wrote:
>>
>>
>> On 5/27/2022 12:38 PM, Dmitry Baryshkov wrote:
>>> On 27/05/2022 21:54, Jessica Zhang wrote:
>>>> Add support for setting MISR registers within the interface
>>>>
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan at quicinc.com>
>>>> ---
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 55
>>>> ++++++++++++++++++++-
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 8 ++-
>>>> 2 files changed, 61 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>> index 3f4d2c6e1b45..29aaeff9eacd 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>>>> @@ -1,5 +1,7 @@
>>>> // SPDX-License-Identifier: GPL-2.0-only
>>>> -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
>>>> +/*
>>>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
>>>> reserved.
>>>> + * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
>>>> */
>>>> #include "dpu_hwio.h"
>>>> @@ -67,6 +69,14 @@
>>>> #define INTF_CFG2_DATABUS_WIDEN BIT(0)
>>>> #define INTF_CFG2_DATA_HCTL_EN BIT(4)
>>>> +#define INTF_MISR_CTRL 0x180
>>>> +#define INTF_MISR_SIGNATURE 0x184
>>>> +#define INTF_MISR_FRAME_COUNT_MASK 0xFF
>>>> +#define INTF_MISR_CTRL_ENABLE BIT(8)
>>>> +#define INTF_MISR_CTRL_STATUS BIT(9)
>>>> +#define INTF_MISR_CTRL_STATUS_CLEAR BIT(10)
>>>> +#define INTF_MISR_CTRL_FREE_RUN_MASK BIT(31)
>>>
>>> I'm tempted to ask to move these bits to some common header. Is there
>>> any other hardware block which uses the same bitfields to control MISR?
>>
>> dpu_hw_lm.c has similar macros here [1] for _ENABLE, _STATUS,
>> _STATUS_CLEAR, and _FREE_RUN_MASK
>>
>> [1]
>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c#L31
>
>
> Please move bit definitions to dpu_hw_util.h. According to what I
> observe, this might be useful.
Noted.
>
>>>> +
>>>> static const struct dpu_intf_cfg *_intf_offset(enum dpu_intf intf,
>>>> const struct dpu_mdss_cfg *m,
>>>> void __iomem *addr,
>>>> @@ -319,6 +329,47 @@ static u32 dpu_hw_intf_get_line_count(struct
>>>> dpu_hw_intf *intf)
>>>> return DPU_REG_READ(c, INTF_LINE_COUNT);
>>>> }
>>>> +static void dpu_hw_intf_setup_misr(struct dpu_hw_intf *intf, bool
>>>> enable, u32 frame_count)
>>>> +{
>>>> + struct dpu_hw_blk_reg_map *c = &intf->hw;
>>>> + u32 config = 0;
>>>> +
>>>> + DPU_REG_WRITE(c, INTF_MISR_CTRL, INTF_MISR_CTRL_STATUS_CLEAR);
>>>> +
>>>> + /* Clear old MISR value (in case it's read before a new value
>>>> is calculated)*/
>>>> + wmb();
>>>> +
>>>> + if (enable) {
>>>> + config = (frame_count & INTF_MISR_FRAME_COUNT_MASK) |
>>>> + INTF_MISR_CTRL_ENABLE | INTF_MISR_CTRL_FREE_RUN_MASK;
>>>> +
>>>> + DPU_REG_WRITE(c, INTF_MISR_CTRL, config);
>>>> + } else {
>>>> + DPU_REG_WRITE(c, INTF_MISR_CTRL, 0);
>>>> + }
>
> This should also be abstracted. Please merge these functions with LM's
> and add corresponding helpers to dpu_hw_util.c
Good idea.
>
>>>> +}
>>>> +
>>>> +static int dpu_hw_intf_collect_misr(struct dpu_hw_intf *intf, u32
>>>> *misr_value)
>>>> +{
>>>> + struct dpu_hw_blk_reg_map *c = &intf->hw;
>>>> + u32 ctrl = 0;
>>>> +
>>>> + if (!misr_value)
>>>> + return -EINVAL;
>>>> +
>>>> + ctrl = DPU_REG_READ(c, INTF_MISR_CTRL);
>>>> +
>>>> + if (!(ctrl & INTF_MISR_CTRL_ENABLE))
>>>> + return -ENODATA;
>
> As the users of collect_misr() are going to ignore the -ENODATA, I think
> it would be worth changing this to set *misr_value to 0 and return 0
> here. It would reduce the amount of boilerplate code.
0 might be a valid misr_value so it might be better to not write it to
the debugfs. IMO we should also avoid writing to the debugfs if the misr
is disabled -- that way we won't be spamming the debugfs with
meaningless entries.
Thanks,
Jessica Zhang
>
>>>> +
>>>> + if (!(ctrl & INTF_MISR_CTRL_STATUS))
>>>> + return -EINVAL;
>>>> +
>>>> + *misr_value = DPU_REG_READ(c, INTF_MISR_SIGNATURE);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
>>>> unsigned long cap)
>>>> {
>>>> @@ -329,6 +380,8 @@ static void _setup_intf_ops(struct
>>>> dpu_hw_intf_ops *ops,
>>>> ops->get_line_count = dpu_hw_intf_get_line_count;
>>>> if (cap & BIT(DPU_INTF_INPUT_CTRL))
>>>> ops->bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk;
>>>> + ops->setup_misr = dpu_hw_intf_setup_misr;
>>>> + ops->collect_misr = dpu_hw_intf_collect_misr;
>>>> }
>>>> struct dpu_hw_intf *dpu_hw_intf_init(enum dpu_intf idx,
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>>> index 7b2d96ac61e8..8d0e7b509260 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>>>> @@ -1,5 +1,7 @@
>>>> /* SPDX-License-Identifier: GPL-2.0-only */
>>>> -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
>>>> +/*
>>>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights
>>>> reserved.
>>>> + * Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.
>>>> */
>>>> #ifndef _DPU_HW_INTF_H
>>>> @@ -57,6 +59,8 @@ struct intf_status {
>>>> * @ get_line_count: reads current vertical line counter
>>>> * @bind_pingpong_blk: enable/disable the connection with pingpong
>>>> which will
>>>> * feed pixels to this interface
>>>> + * @setup_misr: enable/disable MISR
>>>> + * @collect_misr: read MISR signature
>>>> */
>>>> struct dpu_hw_intf_ops {
>>>> void (*setup_timing_gen)(struct dpu_hw_intf *intf,
>>>> @@ -77,6 +81,8 @@ struct dpu_hw_intf_ops {
>>>> void (*bind_pingpong_blk)(struct dpu_hw_intf *intf,
>>>> bool enable,
>>>> const enum dpu_pingpong pp);
>>>> + void (*setup_misr)(struct dpu_hw_intf *intf, bool enable, u32
>>>> frame_count);
>>>> + int (*collect_misr)(struct dpu_hw_intf *intf, u32 *misr_value);
>>>> };
>>>> struct dpu_hw_intf {
>>>
>>>
>>> --
>>> With best wishes
>>> Dmitry
>
>
> --
> With best wishes
> Dmitry
More information about the Freedreno
mailing list