mbox series

[RFC,0/7] drm/msm/dpu: Implement tearcheck support on INTF block

Message ID 20221231215006.211860-1-marijn.suijten@somainline.org
Headers show
Series drm/msm/dpu: Implement tearcheck support on INTF block | expand

Message

Marijn Suijten Dec. 31, 2022, 9:49 p.m. UTC
Since DPU 5.0.0 the TEARCHECK registers and interrupts moved out of the
PINGPONG block and into the INTF.  Implement the necessary callbacks in
the INTF block, and use these callbacks together with the INTF_TEAR
interrupts.  Additionally, disable previous register writes and remove
unused interrupts in the PINGPONG and MDP_TOP blocks for these newer
platforms.

With these patches the devices on DPU >= 5.0.0 listed below now update
their panels at 60fps without tearing (nor sluggishness), and without
repeated timeouts in dmesg.

Tested on the following devices with command-mode panels and TE pins:

- Sony Xperia XZ3 (sdm845, DPU 4.0.0, cmdmode panel): no regressions on
  PINGPONG TE;
- Sony Xperia 5 (sm8150, DPU 5.0.0);
- Sony Xperia 10 II (sm6125, DPU 5.0.4).

Konrad Dybcio (1):
  drm/msm/dpu: Move dpu_hw_{tear_check,pp_vsync_info} to dpu_hw_mdss.h

Marijn Suijten (6):
  drm/msm/dpu: Remove unused INTF0 interrupt mask from sm6115/qcm2290
  drm/msm/dpu: Disable pingpong TE on DPU 5.0.0 and above
  drm/msm/dpu: Disable MDP vsync source selection on DPU 5.0.0 and above
  drm/msm/dpu: Document and enable TEAR interrupts on DSI interfaces
  drm/msm/dpu: Implement tearcheck support on INTF block
  drm/msm/dpu: Remove intr_rdptr from DPU >= 5.0.0 pingpong config

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   |  11 +
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  10 +-
 .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 108 +++++++--
 .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c    | 181 +++++++++------
 .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    |   7 +-
 .../gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c |  12 +
 .../gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h |   2 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c   | 206 ++++++++++++++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h   |  29 +++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h   |  48 ++++
 .../gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c   |  18 +-
 .../gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.h   |  22 --
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c    |  52 +++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h      |   3 +
 14 files changed, 570 insertions(+), 139 deletions(-)

--
2.39.0

Comments

Dmitry Baryshkov Jan. 1, 2023, 4:18 a.m. UTC | #1
On 31/12/2022 23:50, Marijn Suijten wrote:
> Neither of these SoCs has INTF0, they only have a DSI interface on index
> 1.  Stop enabling an interrupt that can't fire.
> 
> Fixes: 3581b7062cec ("drm/msm/disp/dpu1: add support for display on SM6115")
> Fixes: 5334087ee743 ("drm/msm: add support for QCM2290 MDSS")
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Dmitry Baryshkov Jan. 1, 2023, 12:27 p.m. UTC | #2
On Sat, 31 Dec 2022 at 23:50, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> Now that newer DPU platforms use a readpointer-done interrupt on the
> INTF block, stop providing the unused interrupt on the PINGPONG block.
>
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Dmitry Baryshkov Jan. 1, 2023, 12:28 p.m. UTC | #3
On Sat, 31 Dec 2022 at 23:52, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> On 2022-12-31 22:50:02, Marijn Suijten wrote:
> > Since hardware revision 5.0.0 the TE configuration moved out of the
> > PINGPONG block into the INTF block, including vsync source selection
> > that was previously part of MDP top.  Writing to the MDP_VSYNC_SEL
> > register has no effect anymore and is omitted downstream via the
> > DPU/SDE_MDP_VSYNC_SEL feature flag.  This flag is only added to INTF
> > blocks used by hardware prior to 5.0.0.
> >
> > The code that writes to these registers in the INTF block will follow in
> > subsequent patches.
> >
> > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > ---
> >  .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c    | 33 ++++++++++--
> >  .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    |  1 +
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c    | 52 +++++++++++++------
> >  3 files changed, 66 insertions(+), 20 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 39d4b293710c..1cfe94494135 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > @@ -407,7 +407,7 @@ static const struct dpu_mdp_cfg msm8998_mdp[] = {
> >       {
> >       .name = "top_0", .id = MDP_TOP,
> >       .base = 0x0, .len = 0x458,
> > -     .features = 0,
> > +     .features = BIT(DPU_MDP_VSYNC_SEL),
> >       .highest_bank_bit = 0x2,
> >       .clk_ctrls[DPU_CLK_CTRL_VIG0] = {
> >                       .reg_off = 0x2AC, .bit_off = 0},
> > @@ -436,7 +436,7 @@ static const struct dpu_mdp_cfg sdm845_mdp[] = {
> >       {
> >       .name = "top_0", .id = MDP_TOP,
> >       .base = 0x0, .len = 0x45C,
> > -     .features = BIT(DPU_MDP_AUDIO_SELECT),
> > +     .features = BIT(DPU_MDP_AUDIO_SELECT) | BIT(DPU_MDP_VSYNC_SEL),
> >       .highest_bank_bit = 0x2,
> >       .clk_ctrls[DPU_CLK_CTRL_VIG0] = {
> >                       .reg_off = 0x2AC, .bit_off = 0},
> > @@ -512,6 +512,31 @@ static const struct dpu_mdp_cfg sm6115_mdp[] = {
> >       },
> >  };
> >
> > +static const struct dpu_mdp_cfg sdm8150_mdp[] = {
>
> Sometimes it is only possible to spot such things _after_ sending,
> probably the thing that makes us human :)
>
> sm8150_mdp*, not sdm.
>

With this name fixed:

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Dmitry Baryshkov Jan. 1, 2023, 1:12 p.m. UTC | #4
On 31/12/2022 23:50, Marijn Suijten wrote:
> All SoCs since DPU 5.0.0 (and seemingly up until and including 6.0.0,
> but excluding 7.x.x) have the tear interrupt and control registers moved
> out of the PINGPONG block and into the INTF block.  Wire up the
> necessary interrupts and IRQ masks on all supported hardware.
> 
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
>   .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c    | 78 +++++++++++--------
>   .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    |  6 +-
>   .../gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 12 +++
>   .../gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h |  2 +
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h      |  3 +
>   5 files changed, 68 insertions(+), 33 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 1cfe94494135..b9b9b5b0b615 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> @@ -86,6 +86,15 @@
>   
>   #define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN)
>   
> +#define IRQ_MSM8998_MASK (BIT(MDP_SSPP_TOP0_INTR) | \
> +			 BIT(MDP_SSPP_TOP0_INTR2) | \
> +			 BIT(MDP_SSPP_TOP0_HIST_INTR) | \
> +			 BIT(MDP_INTF0_INTR) | \
> +			 BIT(MDP_INTF1_INTR) | \
> +			 BIT(MDP_INTF2_INTR) | \
> +			 BIT(MDP_INTF3_INTR) | \
> +			 BIT(MDP_INTF4_INTR))
> +
>   #define IRQ_SDM845_MASK (BIT(MDP_SSPP_TOP0_INTR) | \
>   			 BIT(MDP_SSPP_TOP0_INTR2) | \
>   			 BIT(MDP_SSPP_TOP0_HIST_INTR) | \
> @@ -100,13 +109,15 @@
>   #define IRQ_QCM2290_MASK (BIT(MDP_SSPP_TOP0_INTR) | \
>   			 BIT(MDP_SSPP_TOP0_INTR2) | \
>   			 BIT(MDP_SSPP_TOP0_HIST_INTR) | \
> -			 BIT(MDP_INTF1_INTR))
> +			 BIT(MDP_INTF1_INTR) | \
> +			 BIT(MDP_INTF1_TEAR_INTR))
>   
>   #define IRQ_SC7180_MASK (BIT(MDP_SSPP_TOP0_INTR) | \
>   			 BIT(MDP_SSPP_TOP0_INTR2) | \
>   			 BIT(MDP_SSPP_TOP0_HIST_INTR) | \
>   			 BIT(MDP_INTF0_INTR) | \
> -			 BIT(MDP_INTF1_INTR))
> +			 BIT(MDP_INTF1_INTR) | \
> +			 BIT(MDP_INTF1_TEAR_INTR))
>   
>   #define IRQ_SC7280_MASK (BIT(MDP_SSPP_TOP0_INTR) | \
>   			 BIT(MDP_SSPP_TOP0_INTR2) | \
> @@ -120,7 +131,9 @@
>   			 BIT(MDP_SSPP_TOP0_HIST_INTR) | \
>   			 BIT(MDP_INTF0_INTR) | \
>   			 BIT(MDP_INTF1_INTR) | \
> +			 BIT(MDP_INTF1_TEAR_INTR) | \
>   			 BIT(MDP_INTF2_INTR) | \
> +			 BIT(MDP_INTF2_TEAR_INTR) | \
>   			 BIT(MDP_INTF3_INTR) | \
>   			 BIT(MDP_INTF4_INTR))
>   
> @@ -129,7 +142,9 @@
>   			  BIT(MDP_SSPP_TOP0_HIST_INTR) | \
>   			  BIT(MDP_INTF0_INTR) | \
>   			  BIT(MDP_INTF1_INTR) | \
> +			  BIT(MDP_INTF1_TEAR_INTR) | \
>   			  BIT(MDP_INTF2_INTR) | \
> +			  BIT(MDP_INTF2_TEAR_INTR) | \
>   			  BIT(MDP_INTF3_INTR) | \
>   			  BIT(MDP_INTF4_INTR) | \
>   			  BIT(MDP_INTF5_INTR) | \
> @@ -1300,63 +1315,64 @@ static struct dpu_dsc_cfg sdm845_dsc[] = {
>   /*************************************************************
>    * INTF sub blocks config
>    *************************************************************/
> -#define INTF_BLK(_name, _id, _base, _type, _ctrl_id, _progfetch, _features, _reg, _underrun_bit, _vsync_bit) \
> +#define INTF_BLK(_name, _id, _base, _len, _type, _ctrl_id, _progfetch, _features, _reg, _underrun_bit, _vsync_bit, _tear_reg, _tear_rd_ptr_bit) \
>   	{\
>   	.name = _name, .id = _id, \
> -	.base = _base, .len = 0x280, \
> +	.base = _base, .len = _len, \

Please move .len setting to a separate patch, it is not direclty related 
to tear interrupt addition.

>   	.features = _features, \
>   	.type = _type, \
>   	.controller_id = _ctrl_id, \
>   	.prog_fetch_lines_worst_case = _progfetch, \
>   	.intr_underrun = DPU_IRQ_IDX(_reg, _underrun_bit), \
>   	.intr_vsync = DPU_IRQ_IDX(_reg, _vsync_bit), \
> +	.intr_tear_rd_ptr = DPU_IRQ_IDX(_tear_reg, _tear_rd_ptr_bit), \

Initially I added separate _reg and _bit settings because reg was common 
to both interrupts. However now as tear interrups use different reg it 
might be better to first move DPU_IRQ_IDX out of this macro () and then 
to add your tear_rd_ptr_intr as a single intr_idx.

>   	}
>   
>   static const struct dpu_intf_cfg msm8998_intf[] = {
> -	INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 25, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> -	INTF_BLK("intf_1", INTF_1, 0x6A800, INTF_DSI, 0, 25, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> -	INTF_BLK("intf_2", INTF_2, 0x6B000, INTF_DSI, 1, 25, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 28, 29),
> -	INTF_BLK("intf_3", INTF_3, 0x6B800, INTF_HDMI, 0, 25, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 30, 31),
> +	INTF_BLK("intf_0", INTF_0, 0x6A000, 0x268, INTF_DP, 0, 25, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 24, 25, -1, -1),
> +	INTF_BLK("intf_1", INTF_1, 0x6A800, 0x268, INTF_DSI, 0, 25, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 26, 27, -1, -1),
> +	INTF_BLK("intf_2", INTF_2, 0x6B000, 0x268, INTF_DSI, 1, 25, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 28, 29, -1, -1),
> +	INTF_BLK("intf_3", INTF_3, 0x6B800, 0x268, INTF_HDMI, 0, 25, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 30, 31, -1, -1),
>   };
>   
>   static const struct dpu_intf_cfg sdm845_intf[] = {
> -	INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> -	INTF_BLK("intf_1", INTF_1, 0x6A800, INTF_DSI, 0, 24, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> -	INTF_BLK("intf_2", INTF_2, 0x6B000, INTF_DSI, 1, 24, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 28, 29),
> -	INTF_BLK("intf_3", INTF_3, 0x6B800, INTF_DP, 1, 24, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 30, 31),
> +	INTF_BLK("intf_0", INTF_0, 0x6A000, 0x280, INTF_DP, 0, 24, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 24, 25, -1, -1),
> +	INTF_BLK("intf_1", INTF_1, 0x6A800, 0x280, INTF_DSI, 0, 24, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 26, 27, -1, -1),
> +	INTF_BLK("intf_2", INTF_2, 0x6B000, 0x280, INTF_DSI, 1, 24, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 28, 29, -1, -1),
> +	INTF_BLK("intf_3", INTF_3, 0x6B800, 0x280, INTF_DP, 1, 24, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 30, 31, -1, -1),
>   };
>   
>   static const struct dpu_intf_cfg sc7180_intf[] = {
> -	INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, MSM_DP_CONTROLLER_0, 24, INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> -	INTF_BLK("intf_1", INTF_1, 0x6A800, INTF_DSI, 0, 24, INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> +	INTF_BLK("intf_0", INTF_0, 0x6A000, 0x280, INTF_DP, MSM_DP_CONTROLLER_0, 24, INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 24, 25, -1, -1),
> +	INTF_BLK("intf_1", INTF_1, 0x6A800, 0x2b8, INTF_DSI, 0, 24, INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 26, 27, MDP_INTF1_TEAR_INTR, 2),
>   };
>   
>   static const struct dpu_intf_cfg sm8150_intf[] = {
> -	INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, 0, 24, INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> -	INTF_BLK("intf_1", INTF_1, 0x6A800, INTF_DSI, 0, 24, INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> -	INTF_BLK("intf_2", INTF_2, 0x6B000, INTF_DSI, 1, 24, INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 28, 29),
> -	INTF_BLK("intf_3", INTF_3, 0x6B800, INTF_DP, 1, 24, INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 30, 31),
> +	INTF_BLK("intf_0", INTF_0, 0x6A000, 0x280, INTF_DP, 0, 24, INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 24, 25, -1, -1),
> +	INTF_BLK("intf_1", INTF_1, 0x6A800, 0x2b8, INTF_DSI, 0, 24, INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 26, 27, MDP_INTF1_TEAR_INTR, 2),
> +	INTF_BLK("intf_2", INTF_2, 0x6B000, 0x2b8, INTF_DSI, 1, 24, INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 28, 29, MDP_INTF2_TEAR_INTR, 2),
> +	INTF_BLK("intf_3", INTF_3, 0x6B800, 0x280, INTF_DP, 1, 24, INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 30, 31, -1, -1),
>   };
>   
>   static const struct dpu_intf_cfg sc7280_intf[] = {
> -	INTF_BLK("intf_0", INTF_0, 0x34000, INTF_DP, MSM_DP_CONTROLLER_0, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> -	INTF_BLK("intf_1", INTF_1, 0x35000, INTF_DSI, 0, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> -	INTF_BLK("intf_5", INTF_5, 0x39000, INTF_DP, MSM_DP_CONTROLLER_1, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
> +	INTF_BLK("intf_0", INTF_0, 0x34000, 0x280, INTF_DP, MSM_DP_CONTROLLER_0, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25, -1, -1),
> +	INTF_BLK("intf_1", INTF_1, 0x35000, 0x2b8, INTF_DSI, 0, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 26, 27, MDP_INTF1_TEAR_INTR, 2),
> +	INTF_BLK("intf_5", INTF_5, 0x39000, 0x280, INTF_DP, MSM_DP_CONTROLLER_1, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23, -1, -1),
>   };
>   
>   static const struct dpu_intf_cfg sc8180x_intf[] = {
> -	INTF_BLK("intf_0", INTF_0, 0x6A000, INTF_DP, MSM_DP_CONTROLLER_0, 24, INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> -	INTF_BLK("intf_1", INTF_1, 0x6A800, INTF_DSI, 0, 24, INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> -	INTF_BLK("intf_2", INTF_2, 0x6B000, INTF_DSI, 1, 24, INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 28, 29),
> +	INTF_BLK("intf_0", INTF_0, 0x6A000, 0x280, INTF_DP, MSM_DP_CONTROLLER_0, 24, INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 24, 25, -1, -1),
> +	INTF_BLK("intf_1", INTF_1, 0x6A800, 0x2b8, INTF_DSI, 0, 24, INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 26, 27, MDP_INTF1_TEAR_INTR, 2),
> +	INTF_BLK("intf_2", INTF_2, 0x6B000, 0x2b8, INTF_DSI, 1, 24, INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 28, 29, MDP_INTF2_TEAR_INTR, 2),
>   	/* INTF_3 is for MST, wired to INTF_DP 0 and 1, use dummy index until this is supported */
> -	INTF_BLK("intf_3", INTF_3, 0x6B800, INTF_DP, 999, 24, INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 30, 31),
> -	INTF_BLK("intf_4", INTF_4, 0x6C000, INTF_DP, MSM_DP_CONTROLLER_1, 24, INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 20, 21),
> -	INTF_BLK("intf_5", INTF_5, 0x6C800, INTF_DP, MSM_DP_CONTROLLER_2, 24, INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
> +	INTF_BLK("intf_3", INTF_3, 0x6B800, 0x280, INTF_DP, 999, 24, INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 30, 31, -1, -1),
> +	INTF_BLK("intf_4", INTF_4, 0x6C000, 0x280, INTF_DP, MSM_DP_CONTROLLER_1, 24, INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 20, 21, -1, -1),
> +	INTF_BLK("intf_5", INTF_5, 0x6C800, 0x280, INTF_DP, MSM_DP_CONTROLLER_2, 24, INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 22, 23, -1, -1),
>   };
>   
>   static const struct dpu_intf_cfg qcm2290_intf[] = {
> -	INTF_BLK("intf_0", INTF_0, 0x00000, INTF_NONE, 0, 0, 0, 0, 0, 0),
> -	INTF_BLK("intf_1", INTF_1, 0x6A800, INTF_DSI, 0, 24, INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> +	INTF_BLK("intf_0", INTF_0, 0x00000, 0x2b8, INTF_NONE, 0, 0, 0, 0, 0, 0, -1, -1),
> +	INTF_BLK("intf_1", INTF_1, 0x6A800, 0x2b8, INTF_DSI, 0, 24, INTF_SC7180_MASK, MDP_SSPP_TOP0_INTR, 26, 27, MDP_INTF2_TEAR_INTR, 2),
>   };
>   
>   /*************************************************************
> @@ -1849,7 +1865,7 @@ static const struct dpu_mdss_cfg msm8998_dpu_cfg = {
>   	.vbif = msm8998_vbif,
>   	.reg_dma_count = 0,
>   	.perf = &msm8998_perf_data,
> -	.mdss_irqs = IRQ_SM8250_MASK,
> +	.mdss_irqs = IRQ_MSM8998_MASK,
>   };
>   
>   static const struct dpu_mdss_cfg sdm845_dpu_cfg = {
> @@ -1947,7 +1963,7 @@ static const struct dpu_mdss_cfg sm8150_dpu_cfg = {
>   	.reg_dma_count = 1,
>   	.dma_cfg = &sm8150_regdma,
>   	.perf = &sm8150_perf_data,
> -	.mdss_irqs = IRQ_SDM845_MASK,
> +	.mdss_irqs = IRQ_SM8250_MASK,
>   };
>   
>   static const struct dpu_mdss_cfg sc8180x_dpu_cfg = {
> 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 e0e153889ab7..2ea17e4ef3e5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> @@ -638,8 +638,9 @@ struct dpu_dsc_cfg {
>    * @type:              Interface type(DSI, DP, HDMI)
>    * @controller_id:     Controller Instance ID in case of multiple of intf type
>    * @prog_fetch_lines_worst_case	Worst case latency num lines needed to prefetch
> - * @intr_underrun:	index for INTF underrun interrupt
> - * @intr_vsync:	        index for INTF VSYNC interrupt
> + * @intr_underrun:     index for INTF underrun interrupt
> + * @intr_vsync:        index for INTF VSYNC interrupt
> + * @intr_tear_rd_ptr:  index for INTF TEAR_RD_PTR interrupt
>    */
>   struct dpu_intf_cfg  {
>   	DPU_HW_BLK_INFO;
> @@ -648,6 +649,7 @@ struct dpu_intf_cfg  {
>   	u32 prog_fetch_lines_worst_case;
>   	s32 intr_underrun;
>   	s32 intr_vsync;
> +	s32 intr_tear_rd_ptr;
>   };
>   
>   /**
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> index cf1b6d84c18a..044136a97fac 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
> @@ -24,6 +24,8 @@
>   #define MDP_INTF_3_OFF			0x6B800
>   #define MDP_INTF_4_OFF			0x6C000
>   #define MDP_INTF_5_OFF			0x6C800
> +#define MDP_INTF_1_TEAR_OFF		0x6D800
> +#define MDP_INTF_2_TEAR_OFF		0x6D900
>   #define MDP_AD4_0_OFF			0x7C000
>   #define MDP_AD4_1_OFF			0x7D000
>   #define MDP_AD4_INTR_EN_OFF		0x41c
> @@ -99,6 +101,16 @@ static const struct dpu_intr_reg dpu_intr_set[] = {
>   		MDP_INTF_5_OFF+INTF_INTR_EN,
>   		MDP_INTF_5_OFF+INTF_INTR_STATUS
>   	},
> +	[MDP_INTF1_TEAR_INTR] = {
> +		MDP_INTF_1_TEAR_OFF+INTF_INTR_TEAR_CLEAR,
> +		MDP_INTF_1_TEAR_OFF+INTF_INTR_TEAR_EN,
> +		MDP_INTF_1_TEAR_OFF+INTF_INTR_TEAR_STATUS
> +	},
> +	[MDP_INTF2_TEAR_INTR] = {
> +		MDP_INTF_2_TEAR_OFF+INTF_INTR_TEAR_CLEAR,
> +		MDP_INTF_2_TEAR_OFF+INTF_INTR_TEAR_EN,
> +		MDP_INTF_2_TEAR_OFF+INTF_INTR_TEAR_STATUS
> +	},
>   	[MDP_AD4_0_INTR] = {
>   		MDP_AD4_0_OFF + MDP_AD4_INTR_CLEAR_OFF,
>   		MDP_AD4_0_OFF + MDP_AD4_INTR_EN_OFF,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
> index 46443955443c..b447e4a1d9f4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
> @@ -23,6 +23,8 @@ enum dpu_hw_intr_reg {
>   	MDP_INTF3_INTR,
>   	MDP_INTF4_INTR,
>   	MDP_INTF5_INTR,
> +	MDP_INTF1_TEAR_INTR,
> +	MDP_INTF2_TEAR_INTR,
>   	MDP_AD4_0_INTR,
>   	MDP_AD4_1_INTR,
>   	MDP_INTF0_7xxx_INTR,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h
> index c8156ed4b7fb..6bdac515fd54 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hwio.h
> @@ -10,6 +10,9 @@
>   /**
>    * MDP TOP block Register and bit fields and defines
>    */
> +#define INTF_INTR_TEAR_EN               0x000
> +#define INTF_INTR_TEAR_STATUS           0x004
> +#define INTF_INTR_TEAR_CLEAR            0x008
>   #define DISP_INTF_SEL                   0x004
>   #define INTR_EN                         0x010
>   #define INTR_STATUS                     0x014
Konrad Dybcio Jan. 2, 2023, 9:30 a.m. UTC | #5
On 31.12.2022 22:52, Marijn Suijten wrote:
> On 2022-12-31 22:50:02, Marijn Suijten wrote:
>> Since hardware revision 5.0.0 the TE configuration moved out of the
>> PINGPONG block into the INTF block, including vsync source selection
>> that was previously part of MDP top.  Writing to the MDP_VSYNC_SEL
>> register has no effect anymore and is omitted downstream via the
>> DPU/SDE_MDP_VSYNC_SEL feature flag.  This flag is only added to INTF
>> blocks used by hardware prior to 5.0.0.
>>
>> The code that writes to these registers in the INTF block will follow in
>> subsequent patches.
>>
>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
>> ---
>>  .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c    | 33 ++++++++++--
>>  .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    |  1 +
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c    | 52 +++++++++++++------
>>  3 files changed, 66 insertions(+), 20 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 39d4b293710c..1cfe94494135 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> @@ -407,7 +407,7 @@ static const struct dpu_mdp_cfg msm8998_mdp[] = {
>>  	{
>>  	.name = "top_0", .id = MDP_TOP,
>>  	.base = 0x0, .len = 0x458,
>> -	.features = 0,
>> +	.features = BIT(DPU_MDP_VSYNC_SEL),
>>  	.highest_bank_bit = 0x2,
>>  	.clk_ctrls[DPU_CLK_CTRL_VIG0] = {
>>  			.reg_off = 0x2AC, .bit_off = 0},
>> @@ -436,7 +436,7 @@ static const struct dpu_mdp_cfg sdm845_mdp[] = {
>>  	{
>>  	.name = "top_0", .id = MDP_TOP,
>>  	.base = 0x0, .len = 0x45C,
>> -	.features = BIT(DPU_MDP_AUDIO_SELECT),
>> +	.features = BIT(DPU_MDP_AUDIO_SELECT) | BIT(DPU_MDP_VSYNC_SEL),
>>  	.highest_bank_bit = 0x2,
>>  	.clk_ctrls[DPU_CLK_CTRL_VIG0] = {
>>  			.reg_off = 0x2AC, .bit_off = 0},
>> @@ -512,6 +512,31 @@ static const struct dpu_mdp_cfg sm6115_mdp[] = {
>>  	},
>>  };
>>  
>> +static const struct dpu_mdp_cfg sdm8150_mdp[] = {
> 
> Sometimes it is only possible to spot such things _after_ sending,
> probably the thing that makes us human :)
> 
> sm8150_mdp*, not sdm.
> 
> - Marijn
> 
>> +	{
>> +	.name = "top_0", .id = MDP_TOP,
>> +	.base = 0x0, .len = 0x45C,
>> +	.features = BIT(DPU_MDP_AUDIO_SELECT),
>> +	.highest_bank_bit = 0x2,
>> +	.clk_ctrls[DPU_CLK_CTRL_VIG0] = {
>> +			.reg_off = 0x2AC, .bit_off = 0},
Keeping the hex values lowercase would be nice.

Konrad
>> +	.clk_ctrls[DPU_CLK_CTRL_VIG1] = {
>> +			.reg_off = 0x2B4, .bit_off = 0},
>> +	.clk_ctrls[DPU_CLK_CTRL_VIG2] = {
>> +			.reg_off = 0x2BC, .bit_off = 0},
>> +	.clk_ctrls[DPU_CLK_CTRL_VIG3] = {
>> +			.reg_off = 0x2C4, .bit_off = 0},
>> +	.clk_ctrls[DPU_CLK_CTRL_DMA0] = {
>> +			.reg_off = 0x2AC, .bit_off = 8},
>> +	.clk_ctrls[DPU_CLK_CTRL_DMA1] = {
>> +			.reg_off = 0x2B4, .bit_off = 8},
>> +	.clk_ctrls[DPU_CLK_CTRL_CURSOR0] = {
>> +			.reg_off = 0x2BC, .bit_off = 8},
>> +	.clk_ctrls[DPU_CLK_CTRL_CURSOR1] = {
>> +			.reg_off = 0x2C4, .bit_off = 8},
>> +	},
>> +};
>> +
>>  static const struct dpu_mdp_cfg sm8250_mdp[] = {
>>  	{
>>  	.name = "top_0", .id = MDP_TOP,
>> @@ -1901,8 +1926,8 @@ static const struct dpu_mdss_cfg sm6115_dpu_cfg = {
>>  
>>  static const struct dpu_mdss_cfg sm8150_dpu_cfg = {
>>  	.caps = &sm8150_dpu_caps,
>> -	.mdp_count = ARRAY_SIZE(sdm845_mdp),
>> -	.mdp = sdm845_mdp,
>> +	.mdp_count = ARRAY_SIZE(sdm8150_mdp),
>> +	.mdp = sdm8150_mdp,
>>  	.ctl_count = ARRAY_SIZE(sm8150_ctl),
>>  	.ctl = sm8150_ctl,
>>  	.sspp_count = ARRAY_SIZE(sdm845_sspp),
>> 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 3b645d5aa9aa..e0e153889ab7 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> @@ -93,6 +93,7 @@ enum {
>>  	DPU_MDP_UBWC_1_0,
>>  	DPU_MDP_UBWC_1_5,
>>  	DPU_MDP_AUDIO_SELECT,
>> +	DPU_MDP_VSYNC_SEL,
>>  	DPU_MDP_MAX
>>  };
>>  
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
>> index c3110a25a30d..2e699c9ad13c 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
>> @@ -151,28 +151,16 @@ static void dpu_hw_get_danger_status(struct dpu_hw_mdp *mdp,
>>  	status->sspp[SSPP_CURSOR1] = (value >> 26) & 0x3;
>>  }
>>  
>> -static void dpu_hw_setup_vsync_source(struct dpu_hw_mdp *mdp,
>> +static void dpu_hw_setup_vsync_source_v1(struct dpu_hw_mdp *mdp,
>>  		struct dpu_vsync_source_cfg *cfg)
>>  {
>>  	struct dpu_hw_blk_reg_map *c;
>> -	u32 reg, wd_load_value, wd_ctl, wd_ctl2, i;
>> -	static const u32 pp_offset[PINGPONG_MAX] = {0xC, 0x8, 0x4, 0x13, 0x18};
>> +	u32 reg, wd_load_value, wd_ctl, wd_ctl2;
>>  
>> -	if (!mdp || !cfg || (cfg->pp_count > ARRAY_SIZE(cfg->ppnumber)))
>> +	if (!mdp || !cfg)
>>  		return;
>>  
>>  	c = &mdp->hw;
>> -	reg = DPU_REG_READ(c, MDP_VSYNC_SEL);
>> -	for (i = 0; i < cfg->pp_count; i++) {
>> -		int pp_idx = cfg->ppnumber[i] - PINGPONG_0;
>> -
>> -		if (pp_idx >= ARRAY_SIZE(pp_offset))
>> -			continue;
>> -
>> -		reg &= ~(0xf << pp_offset[pp_idx]);
>> -		reg |= (cfg->vsync_source & 0xf) << pp_offset[pp_idx];
>> -	}
>> -	DPU_REG_WRITE(c, MDP_VSYNC_SEL, reg);
>>  
>>  	if (cfg->vsync_source >= DPU_VSYNC_SOURCE_WD_TIMER_4 &&
>>  			cfg->vsync_source <= DPU_VSYNC_SOURCE_WD_TIMER_0) {
>> @@ -219,6 +207,33 @@ static void dpu_hw_setup_vsync_source(struct dpu_hw_mdp *mdp,
>>  	}
>>  }
>>  
>> +static void dpu_hw_setup_vsync_source(struct dpu_hw_mdp *mdp,
>> +		struct dpu_vsync_source_cfg *cfg)
>> +{
>> +	struct dpu_hw_blk_reg_map *c;
>> +	u32 reg, i;
>> +	static const u32 pp_offset[PINGPONG_MAX] = {0xC, 0x8, 0x4, 0x13, 0x18};
>> +
>> +	if (!mdp || !cfg || (cfg->pp_count > ARRAY_SIZE(cfg->ppnumber)))
>> +		return;
>> +
>> +	c = &mdp->hw;
>> +
>> +	reg = DPU_REG_READ(c, MDP_VSYNC_SEL);
>> +	for (i = 0; i < cfg->pp_count; i++) {
>> +		int pp_idx = cfg->ppnumber[i] - PINGPONG_0;
>> +
>> +		if (pp_idx >= ARRAY_SIZE(pp_offset))
>> +			continue;
>> +
>> +		reg &= ~(0xf << pp_offset[pp_idx]);
>> +		reg |= (cfg->vsync_source & 0xf) << pp_offset[pp_idx];
>> +	}
>> +	DPU_REG_WRITE(c, MDP_VSYNC_SEL, reg);
>> +
>> +	dpu_hw_setup_vsync_source_v1(mdp, cfg);
>> +}
>> +
>>  static void dpu_hw_get_safe_status(struct dpu_hw_mdp *mdp,
>>  		struct dpu_danger_safe_status *status)
>>  {
>> @@ -266,7 +281,12 @@ static void _setup_mdp_ops(struct dpu_hw_mdp_ops *ops,
>>  	ops->setup_split_pipe = dpu_hw_setup_split_pipe;
>>  	ops->setup_clk_force_ctrl = dpu_hw_setup_clk_force_ctrl;
>>  	ops->get_danger_status = dpu_hw_get_danger_status;
>> -	ops->setup_vsync_source = dpu_hw_setup_vsync_source;
>> +
>> +	if (cap & BIT(DPU_MDP_VSYNC_SEL))
>> +		ops->setup_vsync_source = dpu_hw_setup_vsync_source;
>> +	else
>> +		ops->setup_vsync_source = dpu_hw_setup_vsync_source_v1;
>> +
>>  	ops->get_safe_status = dpu_hw_get_safe_status;
>>  
>>  	if (cap & BIT(DPU_MDP_AUDIO_SELECT))
>> -- 
>> 2.39.0
>>
Marijn Suijten Jan. 2, 2023, 10:18 a.m. UTC | #6
On 2023-01-02 10:30:58, Konrad Dybcio wrote:
> 
> 
> On 31.12.2022 22:52, Marijn Suijten wrote:
> > On 2022-12-31 22:50:02, Marijn Suijten wrote:
> >> Since hardware revision 5.0.0 the TE configuration moved out of the
> >> PINGPONG block into the INTF block, including vsync source selection
> >> that was previously part of MDP top.  Writing to the MDP_VSYNC_SEL
> >> register has no effect anymore and is omitted downstream via the
> >> DPU/SDE_MDP_VSYNC_SEL feature flag.  This flag is only added to INTF
> >> blocks used by hardware prior to 5.0.0.
> >>
> >> The code that writes to these registers in the INTF block will follow in
> >> subsequent patches.
> >>
> >> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> >> ---
> >>  .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c    | 33 ++++++++++--
> >>  .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    |  1 +
> >>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c    | 52 +++++++++++++------
> >>  3 files changed, 66 insertions(+), 20 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 39d4b293710c..1cfe94494135 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >> @@ -407,7 +407,7 @@ static const struct dpu_mdp_cfg msm8998_mdp[] = {
> >>  	{
> >>  	.name = "top_0", .id = MDP_TOP,
> >>  	.base = 0x0, .len = 0x458,
> >> -	.features = 0,
> >> +	.features = BIT(DPU_MDP_VSYNC_SEL),
> >>  	.highest_bank_bit = 0x2,
> >>  	.clk_ctrls[DPU_CLK_CTRL_VIG0] = {
> >>  			.reg_off = 0x2AC, .bit_off = 0},
> >> @@ -436,7 +436,7 @@ static const struct dpu_mdp_cfg sdm845_mdp[] = {
> >>  	{
> >>  	.name = "top_0", .id = MDP_TOP,
> >>  	.base = 0x0, .len = 0x45C,
> >> -	.features = BIT(DPU_MDP_AUDIO_SELECT),
> >> +	.features = BIT(DPU_MDP_AUDIO_SELECT) | BIT(DPU_MDP_VSYNC_SEL),
> >>  	.highest_bank_bit = 0x2,
> >>  	.clk_ctrls[DPU_CLK_CTRL_VIG0] = {
> >>  			.reg_off = 0x2AC, .bit_off = 0},
> >> @@ -512,6 +512,31 @@ static const struct dpu_mdp_cfg sm6115_mdp[] = {
> >>  	},
> >>  };
> >>  
> >> +static const struct dpu_mdp_cfg sdm8150_mdp[] = {
> > 
> > Sometimes it is only possible to spot such things _after_ sending,
> > probably the thing that makes us human :)
> > 
> > sm8150_mdp*, not sdm.
> > 
> > - Marijn
> > 
> >> +	{
> >> +	.name = "top_0", .id = MDP_TOP,
> >> +	.base = 0x0, .len = 0x45C,
> >> +	.features = BIT(DPU_MDP_AUDIO_SELECT),
> >> +	.highest_bank_bit = 0x2,
> >> +	.clk_ctrls[DPU_CLK_CTRL_VIG0] = {
> >> +			.reg_off = 0x2AC, .bit_off = 0},
> Keeping the hex values lowercase would be nice.

Ack, this was copied verbatim from sdm845_mdp but will cleanup as we go.
Looks like this file has a mixed approach towards lower and uppercase,
when does the normalization patch land?

- Marijn
Konrad Dybcio Jan. 2, 2023, 10:19 a.m. UTC | #7
On 2.01.2023 11:18, Marijn Suijten wrote:
> On 2023-01-02 10:30:58, Konrad Dybcio wrote:
>>
>>
>> On 31.12.2022 22:52, Marijn Suijten wrote:
>>> On 2022-12-31 22:50:02, Marijn Suijten wrote:
>>>> Since hardware revision 5.0.0 the TE configuration moved out of the
>>>> PINGPONG block into the INTF block, including vsync source selection
>>>> that was previously part of MDP top.  Writing to the MDP_VSYNC_SEL
>>>> register has no effect anymore and is omitted downstream via the
>>>> DPU/SDE_MDP_VSYNC_SEL feature flag.  This flag is only added to INTF
>>>> blocks used by hardware prior to 5.0.0.
>>>>
>>>> The code that writes to these registers in the INTF block will follow in
>>>> subsequent patches.
>>>>
>>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
>>>> ---
>>>>  .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c    | 33 ++++++++++--
>>>>  .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    |  1 +
>>>>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c    | 52 +++++++++++++------
>>>>  3 files changed, 66 insertions(+), 20 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 39d4b293710c..1cfe94494135 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>> @@ -407,7 +407,7 @@ static const struct dpu_mdp_cfg msm8998_mdp[] = {
>>>>  	{
>>>>  	.name = "top_0", .id = MDP_TOP,
>>>>  	.base = 0x0, .len = 0x458,
>>>> -	.features = 0,
>>>> +	.features = BIT(DPU_MDP_VSYNC_SEL),
>>>>  	.highest_bank_bit = 0x2,
>>>>  	.clk_ctrls[DPU_CLK_CTRL_VIG0] = {
>>>>  			.reg_off = 0x2AC, .bit_off = 0},
>>>> @@ -436,7 +436,7 @@ static const struct dpu_mdp_cfg sdm845_mdp[] = {
>>>>  	{
>>>>  	.name = "top_0", .id = MDP_TOP,
>>>>  	.base = 0x0, .len = 0x45C,
>>>> -	.features = BIT(DPU_MDP_AUDIO_SELECT),
>>>> +	.features = BIT(DPU_MDP_AUDIO_SELECT) | BIT(DPU_MDP_VSYNC_SEL),
>>>>  	.highest_bank_bit = 0x2,
>>>>  	.clk_ctrls[DPU_CLK_CTRL_VIG0] = {
>>>>  			.reg_off = 0x2AC, .bit_off = 0},
>>>> @@ -512,6 +512,31 @@ static const struct dpu_mdp_cfg sm6115_mdp[] = {
>>>>  	},
>>>>  };
>>>>  
>>>> +static const struct dpu_mdp_cfg sdm8150_mdp[] = {
>>>
>>> Sometimes it is only possible to spot such things _after_ sending,
>>> probably the thing that makes us human :)
>>>
>>> sm8150_mdp*, not sdm.
>>>
>>> - Marijn
>>>
>>>> +	{
>>>> +	.name = "top_0", .id = MDP_TOP,
>>>> +	.base = 0x0, .len = 0x45C,
>>>> +	.features = BIT(DPU_MDP_AUDIO_SELECT),
>>>> +	.highest_bank_bit = 0x2,
>>>> +	.clk_ctrls[DPU_CLK_CTRL_VIG0] = {
>>>> +			.reg_off = 0x2AC, .bit_off = 0},
>> Keeping the hex values lowercase would be nice.
> 
> Ack, this was copied verbatim from sdm845_mdp but will cleanup as we go.
> Looks like this file has a mixed approach towards lower and uppercase,
> when does the normalization patch land?
Rob was against changing everything in one commit, as that would mess
with git blame, so we settled on preventing adding new uppercase hex
for now (outside of #defines ofc).

Konrad
> 
> - Marijn
Marijn Suijten Jan. 2, 2023, 10:38 a.m. UTC | #8
On 2023-01-01 15:12:35, Dmitry Baryshkov wrote:
> On 31/12/2022 23:50, Marijn Suijten wrote:
> > <snip>
> > -#define INTF_BLK(_name, _id, _base, _type, _ctrl_id, _progfetch, _features, _reg, _underrun_bit, _vsync_bit) \
> > +#define INTF_BLK(_name, _id, _base, _len, _type, _ctrl_id, _progfetch, _features, _reg, _underrun_bit, _vsync_bit, _tear_reg, _tear_rd_ptr_bit) \
> >   	{\
> >   	.name = _name, .id = _id, \
> > -	.base = _base, .len = 0x280, \
> > +	.base = _base, .len = _len, \
> 
> Please move .len setting to a separate patch, it is not direclty related 
> to tear interrupt addition.

It is directly related in that the TE registers reside in the extra
space beyond 0x280, but I can surely make that explicit in a separate
patch.

> >   	.features = _features, \
> >   	.type = _type, \
> >   	.controller_id = _ctrl_id, \
> >   	.prog_fetch_lines_worst_case = _progfetch, \
> >   	.intr_underrun = DPU_IRQ_IDX(_reg, _underrun_bit), \
> >   	.intr_vsync = DPU_IRQ_IDX(_reg, _vsync_bit), \
> > +	.intr_tear_rd_ptr = DPU_IRQ_IDX(_tear_reg, _tear_rd_ptr_bit), \
> 
> Initially I added separate _reg and _bit settings because reg was common 
> to both interrupts. However now as tear interrups use different reg it 
> might be better to first move DPU_IRQ_IDX out of this macro () and then 
> to add your tear_rd_ptr_intr as a single intr_idx.

I assumed as much; then we do get the duplication of _reg but I guess
it's not too bad if the lines are nicely wrapped like in _pp[].  I'll do
so in a separate patch.

- Marijn

<snip>