On 5/4/2023 11:23 AM, Marijn Suijten wrote:
On 2023-05-04 20:53:33, Dmitry Baryshkov wrote:
On Thu, 4 May 2023 at 20:49, Marijn Suijten
<marijn.suijten@xxxxxxxxxxxxxx> wrote:
PP_BLK_TE is no longer there.
marcos -> macros.
On 2023-05-04 09:46:41, Kuogee Hsieh wrote:
At legacy chipsets, it required DPU_PINGPONG_DSC bit be set to indicate
I may have not made this clear, but the comments on patch 2/7
(introducing the DPU_PINGPONG_DSC bit) also apply to this patch: clarify
DPU 7.0.0 exactly in favour of "legacy", which has no definition at all
and changes over time.
pingpong ops functions are required to complete DSC data path setup if
this chipset has DSC hardware block presented. This patch add
DPU_PINGPONG_DSC bit to both PP_BLK and PP_BLK_TE marcos if it has DSC
hardware block presented.
Strictly speaking this patch together with 2/7 is not bisectable, as 2/7
first disables the callbacks for _all_ hardware and then this patch adds
it back by adding the flag to all DPU < 7.0.0 catalog descriptions.
I asked to split these into two patches, but I see your point and
partially agree with it. However if we mix the catalog changes with
functional changes, it is too easy to overlook or misjudge the
functional changes.
I did the same in the INTF TE series for patches that have very little
and/or very obvious functional changes: exactly this combination of
guarding a few callbacks behind a feature bit, and setting that feature
bit on a few specific catalog entries.
As you are correct about bisectability, I'd probably suggest either
having three patches (define flag, update catalog, handle flag in the
driver) or squashing first two patches to have two patches (add flag +
catalog, separate functional changes).
Sure, if you really prefer a split I'd go for two patches:
1. Add the flag to the enum and catalog;
2. Add the ops guard (functional change).
Then don't forget to reword the commit message, following the guidelines
below and the suggestion for 2/7.
- Marijn
Plan sounds good to me.
Marijn, we will wait for a couple of days to post the next rev but would
be hard more than that as we need to pick up other things which are
pending on top of this. Hence would appreciate if you can finish reviews
by then.
To solve that, as we do in other DPU patch-series, just squash this
patch into 2/7. That way you also don't have to spend extra time
rewording this commit message either to match the improvements we made
in 2/7 (for example, you mention that "ops functions are required to
complete DSC data path setup", but those were already available before
2/7, despite sounding as if this is a new thing that was previously
missing entirely).
But please wait at least a couple days before sending v6. I only have a
few hours every day/week but would appreciate to review and test all the
other patches.
Signed-off-by: Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>
---
.../drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h | 16 +++++++--------
.../gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h | 16 +++++++--------
.../gpu/drm/msm/disp/dpu1/catalog/dpu_5_0_sm8150.h | 24 +++++++++++-----------
.../drm/msm/disp/dpu1/catalog/dpu_5_1_sc8180x.h | 24 +++++++++++-----------
.../gpu/drm/msm/disp/dpu1/catalog/dpu_6_0_sm8250.h | 24 +++++++++++-----------
5 files changed, 52 insertions(+), 52 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
index 521cfd5..ef92545 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h
@@ -112,17 +112,17 @@ static const struct dpu_lm_cfg msm8998_lm[] = {
};
static const struct dpu_pingpong_cfg msm8998_pp[] = {
- PP_BLK("pingpong_0", PINGPONG_0, 0x70000, PINGPONG_SDM845_TE2_MASK, 0, sdm845_pp_sblk_te,
- DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR, 8),
+ PP_BLK("pingpong_0", PINGPONG_0, 0x70000, PINGPONG_SDM845_TE2_MASK|BIT(DPU_PINGPONG_DSC),
This should be added to the MASK (add new #define's where necessary).
- Marijn
<snip>
