On 11/1/2023 2:23 PM, Dmitry Baryshkov wrote:
On Wed, 1 Nov 2023 at 21:56, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
On 11/1/2023 12:39 PM, Dmitry Baryshkov wrote:
On Wed, 1 Nov 2023 at 20:43, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
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.
I'm trying to eliminate them, because they cause more confusion than
the bonuses.
Currently we have QSEED3 / 3LITE / 4, which are somewhat compatible.
We are aiming to support QSEED2 and RGB, which are incompatible with
the QSEED3/3lite/4 family programming sequence. So they get their own
feature bits (DPU_SSPP_SCALER_QSEED2 and DPU_SSPP_SCALER_RGB).
QSEED5-to-be will either be compatible with QSEED3 (and thus can fall
into the same bucket) or it will be a different kind of scaler (and
will get its own feature).
I'm not a fan of DPU_SSPP_SCALER_QSEED3LITE/QSEED4 and I'd like to
remove those two bits for the following reasons:
- We already encode this info into the scaler version. How should
driver behave it it has e.g. version 3.1, but DPU_SSPP_SCALER_QSEED3?
Or vice versa: version 1.2 but QSEED4 feature bit? Having a single
QSEED3 removes this issue.
- Adding QSEED5-compatible-with-QSEED3 requires changing several
places which deal with the feature bits and the per-version setup
sequence. This seems like an overkill. It is easy to miss one of the
places and thus end up with the half-supported scaler
I admit, it might not be ideal to use QSEED3 for all scaler versions.
I'm open to suggestions on the better name for this feature bit. But I
have no doubts that there should be a single feature bit for all
QSEED3/3LITE/4 scalers.
hmmm, the concern i had was that from the version the driver doesnt seem
to know which qseed it is as you rightly pointed out in your earlier
response with the examples of sdm845, msm8998 etc.
It needs this feature bit to know which qseed version it is to use the
correct scaler function. If you remove the other two places below, this
will be the only one left right?
I was thinking of solving this problem with something like
QSEED3_3LITE_4 but then this is not scalable if QSEED5 is also a variant
of QSEED3.
After we remove below 2 places, are there more places where we test
these feature bits?
Hmm, true, this is the only place enumerating them.
One thing we can perhaps do is move all this feature bit handling to one
function like dpu_scaler_is_qseed3_compatible() and move these feature
bits there. That way you will have only one place to change for all the
code.
What about renaming QSEED3 to QSEED3_COMPATIBLE then? This would leave
us with RGB, QSEED2, QSEED3_COMPATIBLE. The QSEED5-to-be will either
be added as a new entry (and a new scaler function) or it will fall
into the QSEED3_COMPATIBLE bucket.
I'd really like to remove any chance of confusion between QSEEDn and
the scaler_block.version. I think we already had that wrong in several
catalog entries, so let's not walk twice into the same water.
Ok thats fine. Lets go ahead with QSEED3_COMPATIBLE as that aligns with
the expectation that we will use the scaler3_lut for everything thats
compatible with QSEED3.
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 {
