Re: [Freedreno] [PATCH v3] drm/msm/dpu: always program DSC active bits

From: Abhinav Kumar
Date: Thu Apr 20 2023 - 12:40:45 EST


Hi Dmitry / Marijn

On 4/20/2023 7:03 AM, Dmitry Baryshkov wrote:
On 15/04/2023 02:02, Marijn Suijten wrote:
On 2023-04-14 09:46:17, Kuogee Hsieh wrote:
In current code, the dsc active bits are set only if the cfg->dsc is set.

This is the old sentence from v1 again, did you accidentally send the
wrong patch as the improvements from v2 are missing?

However, for displays which are hot-pluggable, there can be a use-case
of disconnecting a DSC supported sink and connecting a non-DSC sink.

For those cases we need to clear DSC active bits during teardown.

At least teardown is one word again, v2 had "tear down" which is wrong.

As discuss at [1], clear DSC active bit will handled at reset_intf_cfg()

discussed* as pointed out by Dmitry, and make it clear that this is
about clearing CTL_DSC_ACTIVE (and CTL_DSC_FLUSH?) specifically.  Once
that is moved to reset_intf_cfg(), this patch should be reverted as
there is no need to write the registers once again when cfg->dsc equals
0.

Kuogee, can we please get a proper v4? With all the relevant changes from v2, with the changelog, etc.

Otherwise the present Reviewed-by tags are just incorrect.


After looking into the DPU DSC changes internally which will be posted today/tomm, that piece of code is again touching this block, so I am now also not convinced this change should be made right now because it was again touching flush programming, so that again leaves only the active bits which as Marijn mentioned will be applicable for a use-case only with hot-pluggable display which we dont have till we land DP DSC.

I think this is what we will do:

-> post DPU DSC changes
-> post the patch to improve reset_intf_cfg() to handle DSC
-> post rest of DP DSC changes
-> post rest of DSI DSC changes

I think that way, we all are aligned.

Apologies for posting this patch a bit ahead of time but if i had foreseen that DPU DSC changes will again touch the flush code, i would have held this patch back too.


- Marijn

Signed-off-by: Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>
Fixes: 77f6da90487c ("drm/msm/disp/dpu1: Add DSC support in hw_ctl")
Reviewed-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
Reviewed-by: Marijn Suijten <marijn.suijten@xxxxxxxxxxxxxx>

[1] https://lore.kernel.org/linux-arm-msm/ec045d6b-4ffd-0f8c-4011-8db45edc6978@xxxxxxxxxxx/
---
  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 7 +++----
  1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
index bbdc95c..88e4efe 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
@@ -541,10 +541,9 @@ static void dpu_hw_ctl_intf_cfg_v1(struct dpu_hw_ctl *ctx,
      if (cfg->merge_3d)
          DPU_REG_WRITE(c, CTL_MERGE_3D_ACTIVE,
                    BIT(cfg->merge_3d - MERGE_3D_0));
-    if (cfg->dsc) {
-        DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX);
-        DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc);
-    }
+
+    DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, DSC_IDX);
+    DPU_REG_WRITE(c, CTL_DSC_ACTIVE, cfg->dsc);
  }
  static void dpu_hw_ctl_intf_cfg(struct dpu_hw_ctl *ctx,
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project