On Fri, 3 Jun 2022 at 04:00, Jessica Zhang <quic_jesszhan@xxxxxxxxxxx> wrote: > > > > 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@xxxxxxxxxxx> > >>>> --- > >>>> 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. Ack, true. Then I'd ask to change encoder code to skip crcs entries corresponding to the phys_encs with no hw_intf bound. -- With best wishes Dmitry
