Re: [PATCH v3 2/3] drm/msm/dpu: use dpu core's major version to enable data compress

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

 





On 6/29/2023 8:22 PM, Dmitry Baryshkov wrote:
On 30/06/2023 06:07, Abhinav Kumar wrote:


On 6/29/2023 5:20 PM, Dmitry Baryshkov wrote:
On 29/06/2023 22:29, Abhinav Kumar wrote:
Instead of using a feature bit to decide whether to enable data
compress or not for DSC use-cases, use dpu core's major version instead.
This will avoid defining feature bits for every bit level details of
registers.

Also, rename the intf's enable_compression() op to program_datapath()
and allow it to accept a struct intf_dpu_datapath_cfg to program
all the bits at once. This can be re-used by widebus later on as
well as it touches the same register.

Two separate commits please, because...


I thought of it but it seemed better together and was the only way I could think of. Please see below.

Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
---
  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c |  9 +++++++--
  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c      |  9 +++++----
  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h      | 16 ++++++++++++++--
  3 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index b856c6286c85..f4e15b4c4cc9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -50,6 +50,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
              to_dpu_encoder_phys_cmd(phys_enc);
      struct dpu_hw_ctl *ctl;
      struct dpu_hw_intf_cfg intf_cfg = { 0 };
+    struct dpu_kms *dpu_kms = phys_enc->dpu_kms;
      ctl = phys_enc->hw_ctl;
      if (!ctl->ops.setup_intf_cfg)
@@ -68,8 +69,12 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
                  phys_enc->hw_intf,
                  phys_enc->hw_pp->idx);
-    if (intf_cfg.dsc != 0 && phys_enc->hw_intf->ops.enable_compression)
-        phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
+    if (intf_cfg.dsc != 0 && dpu_kms->catalog->core_major_version >= 0x7) {

... because this becomes incorrect. The datapath should be programmed in all the cases, if there is a corresponding callback. intf_cfg.dsc should be used as a condition to set datapath_cfg.


The issue is that today we do not have dpu_mdss_cfg as part of dpu_hw_intf nor _setup_intf_ops because all of those have been dropped with some rework or cleanup.

Pass dpu_mdss_cfg to dpu_hw_intf_init(). It was removed as a cleanup, now we can reintroduce it.


Thanks, that will address all these concerns.

I wanted to get agreement before re-introducing it and also make sure there was no other way.



Ideally even I would like to assign this op only for core_rev >=7 but that information is no longer available. We would have to start passing the major and minor versions to _setup_intf_ops() to go with that approach. So without making all of those changes, the only way I had was to assign the op unconditionally but call it only for major_rev >= 7.

Passing core_rev to the op itself so that we can write the register only for core_rev >=7 is an option but then what if some bits start becoming usable only after minor rev. then we will have to start passing major and minor rev to program_datapath too. Again getting little messy.

I am open to ideas to achieve the goal of assigning this op only for core_rev >=7 other than what I wrote above.


+        struct intf_dpu_datapath_cfg datapath_cfg = { 0 };

No need for `0' in the init, empty braces would be enough.


ack.

+
+        datapath_cfg.data_compress = true;
+        phys_enc->hw_intf->ops.program_datapath(phys_enc->hw_intf, &datapath_cfg);
+    }
  }
  static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int irq_idx) 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 5b0f6627e29b..85333df08fbc 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -513,11 +513,13 @@ static void dpu_hw_intf_disable_autorefresh(struct dpu_hw_intf *intf,
  }
-static void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
+static void dpu_hw_intf_program_datapath(struct dpu_hw_intf *ctx,
+                     struct intf_dpu_datapath_cfg *datapath_cfg)
  {
      u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2);
-    intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
+    if (datapath_cfg->data_compress)
+        intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
      DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
  }
@@ -543,8 +545,7 @@ static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
          ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh;
      }
-    if (cap & BIT(DPU_INTF_DATA_COMPRESS))
-        ops->enable_compression = dpu_hw_intf_enable_compression;
+    ops->program_datapath = dpu_hw_intf_program_datapath;

The `core_major_version >= 7' should either be here or in the callback itself.


Yes, ideally I would like it like that but please see above why I couldnt do it.

  }
  struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
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 99e21c4137f9..f736dca38463 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
@@ -48,6 +48,11 @@ struct intf_status {
      u32 line_count;        /* current line count including blanking */
  };
+struct intf_dpu_datapath_cfg {
+    u8 data_compress;    /* enable data compress between dpu and dsi */
+    /* can be expanded for other programmable bits */
+};

I'd say, dpu_datapath is too generic. What about  intf_cmd_mode_cfg?


The goal was to keep it generic. Its actually the handshake between DPU and interface datapath so I chose that name.

Do you have plans of using it for the video mode?


No because we didnt want to touch the video mode path as that was discussed in the widebus series already.

Ok, I am fine with intf_cmd_mode_cfg in that case.



This is not specific to command mode and intf_cfg is already there so I chose that one :)

+
  /**
   * struct dpu_hw_intf_ops : Interface to the interface Hw driver functions    *  Assumption is these functions will be called after clocks are enabled
@@ -70,7 +75,7 @@ struct intf_status {
   * @get_autorefresh:            Retrieve autorefresh config from hardware    *                              Return: 0 on success, -ETIMEDOUT on timeout    * @vsync_sel:                  Select vsync signal for tear-effect configuration
- * @enable_compression:         Enable data compression
+ * @program_datapath:           Program the DPU to interface datapath for relevant chipsets
   */
  struct dpu_hw_intf_ops {
      void (*setup_timing_gen)(struct dpu_hw_intf *intf,
@@ -108,7 +113,14 @@ struct dpu_hw_intf_ops {
       */
      void (*disable_autorefresh)(struct dpu_hw_intf *intf, uint32_t encoder_id, u16 vdisplay);
-    void (*enable_compression)(struct dpu_hw_intf *intf);
+    /**
+     * Program the DPU to intf datapath by specifying
+     * which of the settings need to be programmed for
+     * use-cases which need DPU-intf handshake such as
+     * widebus, compression etc.

This is not a valid kerneldoc.


hmmm ... ok so just // ?

I referred disable_autorefresh from above and did the same.

+     */
+    void (*program_datapath)(struct dpu_hw_intf *intf,
+                 struct intf_dpu_datapath_cfg *datapath_cfg);
  };
  struct dpu_hw_intf {





[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