Re: [Freedreno] [PATCH v2 2/2] drm/msm/dpu: Add SC8180x to hw catalog

Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> · Tue, 15 Feb 2022 23:19:55 -0800

On 2/15/2022 9:14 PM, Bjorn Andersson wrote:
On Tue 15 Feb 20:38 CST 2022, Abhinav Kumar wrote:



On 2/15/2022 6:14 PM, Bjorn Andersson wrote:
On Tue 15 Feb 11:42 CST 2022, Abhinav Kumar wrote:



On 2/15/2022 9:28 AM, Bjorn Andersson wrote:
On Tue 15 Feb 11:14 CST 2022, Abhinav Kumar wrote:



On 2/14/2022 8:33 PM, Bjorn Andersson wrote:
From: Rob Clark <robdclark@xxxxxxxxxxxx>

Add SC8180x to the hardware catalog, for initial support for the
platform. Due to limitations in the DP driver only one of the four DP
interfaces is left enabled.

The SC8180x platform supports the newly added DPU_INTF_WIDEBUS flag and
the Windows-on-Snapdragon bootloader leaves the widebus bit set, so this
is flagged appropriately to ensure widebus is disabled - for now.

Signed-off-by: Rob Clark <robdclark@xxxxxxxxxxxx>
[bjorn: Reworked intf and irq definitions]
Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
---

Changes since v1:
- Dropped widebus flag

     .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c    | 129 ++++++++++++++++++
     .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h    |   1 +
     drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |   1 +
     drivers/gpu/drm/msm/msm_drv.c                 |   1 +
     4 files changed, 132 insertions(+)

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 aa75991903a6..7ac0fe32df49 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -90,6 +90,17 @@
     			 BIT(MDP_INTF3_INTR) | \
     			 BIT(MDP_INTF4_INTR))
+#define IRQ_SC8180X_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) | \
+			  BIT(MDP_INTF5_INTR) | \
+			  BIT(MDP_AD4_0_INTR) | \
+			  BIT(MDP_AD4_1_INTR))
     #define DEFAULT_PIXEL_RAM_SIZE		(50 * 1024)
     #define DEFAULT_DPU_LINE_WIDTH		2048
@@ -225,6 +236,22 @@ static const struct dpu_caps sm8150_dpu_caps = {
     	.max_vdeci_exp = MAX_VERT_DECIMATION,
     };
+static const struct dpu_caps sc8180x_dpu_caps = {
+	.max_mixer_width = DEFAULT_DPU_OUTPUT_LINE_WIDTH,
+	.max_mixer_blendstages = 0xb,
+	.qseed_type = DPU_SSPP_SCALER_QSEED3,
+	.smart_dma_rev = DPU_SSPP_SMART_DMA_V2, /* TODO: v2.5 */
+	.ubwc_version = DPU_HW_UBWC_VER_30,
+	.has_src_split = true,
+	.has_dim_layer = true,
+	.has_idle_pc = true,
+	.has_3d_merge = true,
+	.max_linewidth = 4096,
+	.pixel_ram_size = DEFAULT_PIXEL_RAM_SIZE,
+	.max_hdeci_exp = MAX_HORZ_DECIMATION,
+	.max_vdeci_exp = MAX_VERT_DECIMATION,
+};
+
     static const struct dpu_caps sm8250_dpu_caps = {
     	.max_mixer_width = DEFAULT_DPU_OUTPUT_LINE_WIDTH,
     	.max_mixer_blendstages = 0xb,
@@ -293,6 +320,31 @@ static const struct dpu_mdp_cfg sc7180_mdp[] = {
     	},
     };
+static const struct dpu_mdp_cfg sc8180x_mdp[] = {
+	{
+	.name = "top_0", .id = MDP_TOP,
+	.base = 0x0, .len = 0x45C,
+	.features = 0,
+	.highest_bank_bit = 0x3,
+	.clk_ctrls[DPU_CLK_CTRL_VIG0] = {
+			.reg_off = 0x2AC, .bit_off = 0},
+	.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,
@@ -861,6 +913,16 @@ static const struct dpu_intf_cfg sc7280_intf[] = {
     	INTF_BLK("intf_5", INTF_5, 0x39000, INTF_DP, MSM_DP_CONTROLLER_1, 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
     };
+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_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),

This is a continued discussion from
https://patchwork.freedesktop.org/patch/474179/.

Shouldnt INTF_5 be marked as INTF_eDP?


Might be, I didn't even know we had an INTF_EDP define...

Is there any reason to distinguish DP and EDP in the DPU?  I see sc7280
doesn't distinguish the DP and EDP interfaces.

Regards,
Bjorn


Like I have mentioned in the other patch, I think we have enough confusion
between eDP and DP with the common driver. Since DPU does have separate
interfaces I think we should fix that.

Regarding sc7280 using INTF_DP, I synced up with Sankeerth. He referred to
your change
https://patchwork.freedesktop.org/patch/457776/?series=92992&rev=5 as it was
posted earlier and ended up using the same INTF_DP macro. So its turning out
to be a cyclical error.


That made me take a second look at the HPG, and sure enough INTF_5 on
SC7280 is connected to a eDP/DP Combo PHY. We have the same setup in
SC8280XP.

In SC8180X, INTF_5 is documented as being connected to a eDP (only) PHY,
so perhaps it makes sense to do it there, but for the others its wrong.


Here you are specifying the controller in the catalog.

No, I'm specifying the type of the INTF. We then use the type of the
intf and the index to match that to a particular DP TX block.

So independent of the PHY thats being used, shouldnt this remain
INTF_eDP?


I don't think it's going to help anyone to say that an interface
connected to a PHY that can be either DP or EDP, should be INTF_EDP.

People are going to make assumptions in the code such as INTF_EDP does
not have audio and then someone designs a board based on SC7280 with DP
output where they expect audio. Or assumptions about HPD, panel etc...

I'm not saying that we have all the details figured out on how that's
going to be controlled, but until there's a reason to distinguish
INTF_DP from INTF_EDP I think we should not make one up. And I don't see
that those differences should be hard coded in the DPU driver.


If it's confusing to people that DP might be driving an EDP output, then
perhaps we can just name it TMDS again? ;)

If you prefer to have TMDS, then like I commented earlier we dont really 
need this change https://patchwork.freedesktop.org/patch/474271/ :)

Whats the benefit of making that change? DRM_ENCODER_TMDS_* can be eDP 
and DP . Then there is no confusion or guess work in the encoder.

DRM_ENCODER_DSI - INTF_DSI
DRM_ENCODER_VIRTUAL - INTF_WB
DRM_ENCODER_TMDS - INTF_DP OR INTF_eDP ( doesnt matter )


Regards,
Bjorn






