| Message ID | 20220527185407.162-3-quic_jesszhan@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Expand CRC to support interface blocks | expand |
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@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? > + > 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); > + } > +} > + > +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; > + > + 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 {
On 2022-05-27 22:38:24, Dmitry Baryshkov wrote: > [..] > > #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? Can we use the "rnndb" register XML and bindings generator for DPU, similar to how it is currently used for Adreno/freedreno and the DSI kernel drivers? - Marijn
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@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 > >> + >> 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); >> + } >> +} >> + >> +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; >> + >> + 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
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@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
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@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
On Fri, 3 Jun 2022 at 04:00, Jessica Zhang <quic_jesszhan@quicinc.com> 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@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. Ack, true. Then I'd ask to change encoder code to skip crcs entries corresponding to the phys_encs with no hw_intf bound.
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) + 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); + } +} + +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; + + 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 {
Add support for setting MISR registers within the interface Signed-off-by: Jessica Zhang <quic_jesszhan@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(-)