[Freedreno] [PATCH 2/3] drm/msm/dpu: Add MISR register support for interface

[Dmitry Baryshkov]

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.

>>> +
>>>   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

>>> +}
>>> +
>>> +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.

>>> +
>>> +    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