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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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.

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



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux