On 7/27/2023 8:26 AM, Dmitry Baryshkov wrote:
On 27/07/2023 18:24, Abhinav Kumar wrote:
On 7/27/2023 1:39 AM, Dmitry Baryshkov wrote:
On Thu, 27 Jul 2023 at 02:20, Abhinav Kumar
<quic_abhinavk@xxxxxxxxxxx> wrote:
On 5/21/2023 10:10 AM, Dmitry Baryshkov wrote:
Switch to using data from MDSS driver to program the SSPP fetch and
UBWC
configuration.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 18 +++++++++++-------
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 7 +++++--
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 16 +++++++++++++++-
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 1 +
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 3 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 2 ++
6 files changed, 36 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
index cf70a9bd1034..bfd82c2921af 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
@@ -8,6 +8,8 @@
#include "dpu_hw_sspp.h"
#include "dpu_kms.h"
+#include "msm_mdss.h"
+
#include <drm/drm_file.h>
#define DPU_FETCH_CONFIG_RESET_VALUE 0x00000087
@@ -308,26 +310,26 @@ static void dpu_hw_sspp_setup_format(struct
dpu_sw_pipe *pipe,
DPU_REG_WRITE(c, SSPP_FETCH_CONFIG,
DPU_FETCH_CONFIG_RESET_VALUE |
ctx->ubwc->highest_bank_bit << 18);
Does this needs to be protected with if ctx->ubwc check?
Yes... And it should have been already.
- switch (ctx->ubwc->ubwc_version) {
- case DPU_HW_UBWC_VER_10:
+ switch (ctx->ubwc->ubwc_enc_version) {
+ case UBWC_1_0:
The values of UBWC_x_x dont match the values of DPU_HW_UBWC_VER_xx.
What was the reason for the catalog to go with DPU_HW_UBWC_VER_xx in
the
catalog for the encoder version in the first place? Because looking at
the registers UBWC_x_x is the correct value.
If we cannot find the reason why, it should be noted in the commit text
that the values we are using did change.
Huh? This is just an enum. It isn't a part of uABI, nor it is written
to the hardware.
The reason is that, this switch case is moving from comparing one set
of values to totally different ones. So atleast that should be noted.
First thing that struck me was this point because the enums UBWC_x_x
and DPU_HW_UBWC_VER_xx dont match.
Do you have any proposed text in mind?
I was doing some checking about this. The issue was that when this enum
was made, it missed using the SDE_HW_UBWC_VER macro
75 enum {
76 DPU_HW_UBWC_VER_10 = 0x100,
77 DPU_HW_UBWC_VER_20 = 0x200,
78 DPU_HW_UBWC_VER_30 = 0x300,
79 DPU_HW_UBWC_VER_40 = 0x400,
80 };
81
so something like this:
183 */
184 enum {
185 SDE_HW_UBWC_VER_10 = SDE_HW_UBWC_VER(0x100),
186 SDE_HW_UBWC_VER_20 = SDE_HW_UBWC_VER(0x200),
187 SDE_HW_UBWC_VER_30 = SDE_HW_UBWC_VER(0x300),
188 SDE_HW_UBWC_VER_40 = SDE_HW_UBWC_VER(0x400),
189 SDE_HW_UBWC_VER_43 = SDE_HW_UBWC_VER(0x431),
190 };
This macro handles that conversion under the hood.
So I would write something like this
"This also corrects the usage of UBWC version which was incorrect from
the beginning because of the enum storing the DPU_HW_UBWC_*** missed out
the conversion to the full UBWC version"
