From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> To: Jessica Zhang <quic_jesszhan@quicinc.com>, freedreno@lists.freedesktop.org Cc: linux-arm-msm@vger.kernel.org, dri-devel@lists.freedesktop.org, robdclark@gmail.com, seanpaul@chromium.org, swboyd@chromium.org, quic_aravindh@quicinc.com, quic_abhinavk@quicinc.com Subject: Re: [PATCH 2/3] drm/msm/dpu: Add MISR register support for interface Date: Fri, 3 Jun 2022 01:31:18 +0300 [thread overview] Message-ID: <0b3574a3-1dfb-842f-e147-c6522fa30f11@linaro.org> (raw) In-Reply-To: <f81a64fc-c80c-2bc9-a9e3-8eb4a1eccf03@quicinc.com> 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 -- With best wishes Dmitry
WARNING: multiple messages have this Message-ID (diff)
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> To: Jessica Zhang <quic_jesszhan@quicinc.com>, freedreno@lists.freedesktop.org Cc: linux-arm-msm@vger.kernel.org, quic_abhinavk@quicinc.com, dri-devel@lists.freedesktop.org, swboyd@chromium.org, seanpaul@chromium.org, quic_aravindh@quicinc.com Subject: Re: [PATCH 2/3] drm/msm/dpu: Add MISR register support for interface Date: Fri, 3 Jun 2022 01:31:18 +0300 [thread overview] Message-ID: <0b3574a3-1dfb-842f-e147-c6522fa30f11@linaro.org> (raw) In-Reply-To: <f81a64fc-c80c-2bc9-a9e3-8eb4a1eccf03@quicinc.com> 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 -- With best wishes Dmitry
next prev parent reply other threads:[~2022-06-02 22:31 UTC|newest] Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-05-27 18:54 [PATCH 0/3] Expand CRC to support interface blocks Jessica Zhang 2022-05-27 18:54 ` Jessica Zhang 2022-05-27 18:54 ` [PATCH 1/3] drm/msm/dpu: Move LM CRC code into separate method Jessica Zhang 2022-05-27 18:54 ` Jessica Zhang 2022-05-27 19:47 ` Dmitry Baryshkov 2022-05-27 19:47 ` Dmitry Baryshkov 2022-05-27 18:54 ` [PATCH 2/3] drm/msm/dpu: Add MISR register support for interface Jessica Zhang 2022-05-27 18:54 ` Jessica Zhang 2022-05-27 19:38 ` Dmitry Baryshkov 2022-05-27 19:38 ` Dmitry Baryshkov 2022-05-27 19:51 ` Marijn Suijten 2022-05-27 19:51 ` Marijn Suijten 2022-05-27 20:11 ` Jessica Zhang 2022-05-27 20:11 ` Jessica Zhang 2022-06-02 22:31 ` Dmitry Baryshkov [this message] 2022-06-02 22:31 ` Dmitry Baryshkov 2022-06-03 1:00 ` Jessica Zhang 2022-06-03 1:00 ` Jessica Zhang 2022-06-03 7:07 ` Dmitry Baryshkov 2022-06-03 7:07 ` Dmitry Baryshkov 2022-05-27 18:54 ` [PATCH 3/3] drm/msm/dpu: Add interface support for CRC debugfs Jessica Zhang 2022-05-27 18:54 ` Jessica Zhang 2022-05-27 19:46 ` Dmitry Baryshkov 2022-05-27 19:46 ` Dmitry Baryshkov 2022-05-27 22:23 ` [Freedreno] " Jessica Zhang 2022-05-27 22:23 ` Jessica Zhang 2022-06-02 22:51 ` Dmitry Baryshkov 2022-06-02 22:51 ` Dmitry Baryshkov 2022-06-03 1:02 ` Jessica Zhang 2022-06-03 1:02 ` Jessica Zhang 2022-06-03 7:02 ` Dmitry Baryshkov 2022-06-03 7:02 ` Dmitry Baryshkov 2022-06-03 23:21 ` Jessica Zhang 2022-06-03 23:21 ` Jessica Zhang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=0b3574a3-1dfb-842f-e147-c6522fa30f11@linaro.org \ --to=dmitry.baryshkov@linaro.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=freedreno@lists.freedesktop.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=quic_abhinavk@quicinc.com \ --cc=quic_aravindh@quicinc.com \ --cc=quic_jesszhan@quicinc.com \ --cc=robdclark@gmail.com \ --cc=seanpaul@chromium.org \ --cc=swboyd@chromium.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.