On 10/31/2023 1:19 AM, Dmitry Baryshkov wrote:
On Mon, 30 Oct 2023 at 22:24, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
On 10/6/2023 6:14 AM, Dmitry Baryshkov wrote:
Three different features, DPU_SSPP_SCALER_QSEED3, QSEED3LITE and QSEED4
are all related to different versions of the same HW scaling block.
Corresponding driver parts use scaler_blk.version to identify the
correct way to program the hardware. In order to simplify the driver
codepath, merge these three feature bits.
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 4 ++--
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 6 +-----
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 9 ++-------
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 3 +--
4 files changed, 6 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index 32c396abf877..eb867c8123d7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -31,10 +31,10 @@
(VIG_SDM845_MASK | BIT(DPU_SSPP_SMART_DMA_V2))
#define VIG_SC7180_MASK \
- (VIG_MASK | BIT(DPU_SSPP_QOS_8LVL) | BIT(DPU_SSPP_SCALER_QSEED4))
+ (VIG_MASK | BIT(DPU_SSPP_QOS_8LVL) | BIT(DPU_SSPP_SCALER_QSEED3))
#define VIG_SM6125_MASK \
- (VIG_MASK | BIT(DPU_SSPP_QOS_8LVL) | BIT(DPU_SSPP_SCALER_QSEED3LITE))
+ (VIG_MASK | BIT(DPU_SSPP_QOS_8LVL) | BIT(DPU_SSPP_SCALER_QSEED3))
This merging is coming at a cost of inaccuracy. We are marking sc7180
and sm6125 as scaler_qseed3. But they are not. Let me know what you
think of below idea instead.
#define VIG_SC7180_MASK_SDMA \
(VIG_SC7180_MASK | BIT(DPU_SSPP_SMART_DMA_V2))
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
index fc5027b0123a..ba262b3f0bdc 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -51,9 +51,7 @@ enum {
/**
* SSPP sub-blocks/features
* @DPU_SSPP_SCALER_QSEED2, QSEED2 algorithm support
- * @DPU_SSPP_SCALER_QSEED3, QSEED3 alogorithm support
- * @DPU_SSPP_SCALER_QSEED3LITE, QSEED3 Lite alogorithm support
- * @DPU_SSPP_SCALER_QSEED4, QSEED4 algorithm support
+ * @DPU_SSPP_SCALER_QSEED3, QSEED3 alogorithm support (also QSEED3LITE and QSEED4)
* @DPU_SSPP_SCALER_RGB, RGB Scaler, supported by RGB pipes
* @DPU_SSPP_CSC, Support of Color space converion
* @DPU_SSPP_CSC_10BIT, Support of 10-bit Color space conversion
@@ -72,8 +70,6 @@ enum {
enum {
DPU_SSPP_SCALER_QSEED2 = 0x1,
DPU_SSPP_SCALER_QSEED3,
- DPU_SSPP_SCALER_QSEED3LITE,
- DPU_SSPP_SCALER_QSEED4,
DPU_SSPP_SCALER_RGB,
DPU_SSPP_CSC,
DPU_SSPP_CSC_10BIT,
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 7e9c87088e17..d1b70cf72eef 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c
@@ -594,9 +594,7 @@ static void _setup_layer_ops(struct dpu_hw_sspp *c,
test_bit(DPU_SSPP_SMART_DMA_V2, &c->cap->features))
c->ops.setup_multirect = dpu_hw_sspp_setup_multirect;
- if (test_bit(DPU_SSPP_SCALER_QSEED3, &features) ||
- test_bit(DPU_SSPP_SCALER_QSEED3LITE, &features) ||
- test_bit(DPU_SSPP_SCALER_QSEED4, &features))
+ if (test_bit(DPU_SSPP_SCALER_QSEED3, &features))
c->ops.setup_scaler = _dpu_hw_sspp_setup_scaler3;
Can we just do sblk->scaler_blk.version >= 0x3000 instead of this
merging? That way you can still drop those enums without inaccuracy.
No. QSEED3 from sdm845 has version 1.3, msm8998, sdm660 and sdm630
have version 1.2.
Ah got it.
HW versioning is getting harder to generalize with the example you have
given. In my opinion, in that case lets keep these enums intact because
we dont have any other way of knowing the Qseed version otherwise and in
terms of LOC, we are not really saving so much in this change.
In the prev change I agreed because along with the name and the version,
we could still interpret the version but with compressing the enums into
just QSEED3, this becomes very confusing. So now, in the future if we
have QSEED5 HW, we will have to change this anyway as its LUT
programming can change. So we need this distinction.
The below two changes seem fine and that way we are eliminating the
usages of the enum and we will end up with only one place using this.
if (test_bit(DPU_SSPP_CDP, &features))
@@ -629,10 +627,7 @@ int _dpu_hw_sspp_init_debugfs(struct dpu_hw_sspp *hw_pipe, struct dpu_kms *kms,
cfg->len,
kms);
- if (cfg->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
- cfg->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
- cfg->features & BIT(DPU_SSPP_SCALER_QSEED2) ||
- cfg->features & BIT(DPU_SSPP_SCALER_QSEED4))
+ if (sblk->scaler_blk.len)
This part seems fine.
dpu_debugfs_create_regset32("scaler_blk", 0400,
debugfs_root,
sblk->scaler_blk.base + cfg->base,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 43135894263c..ba3ee4ba25b3 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -438,8 +438,7 @@ static void _dpu_plane_setup_scaler3(struct dpu_hw_sspp *pipe_hw,
scale_cfg->src_height[i] /= chroma_subsmpl_v;
}
- if (pipe_hw->cap->features &
- BIT(DPU_SSPP_SCALER_QSEED4)) {
+ if (pipe_hw->cap->sblk->scaler_blk.version >= 0x3000) {
This is fine too.
scale_cfg->preload_x[i] = DPU_QSEED4_DEFAULT_PRELOAD_H;
scale_cfg->preload_y[i] = DPU_QSEED4_DEFAULT_PRELOAD_V;
} else {
