2023-04-19 14:42:25

by Arnaud Vrac

[permalink] [raw]
Subject: [PATCH 00/11] drm/msm/dpu: tweaks for better hardware resources allocation

This series include misc fixes related to hardware resource allocations
in the msm dpu driver, some specifically for msm8998 (including hw
catalog fixes and cursor sspp support for cursor planes, instead of
using Smart DMA pipes).

This series has been tested on msm8998 with additional patches to enable
hdmi support.

The following modetest example command works now; 8 planes can be
displayed simultaneously on msm8998 in 1080p, including a cursor plane,
using a single LM:

modetest -Mmsm -a \
-s 32:1920x1080-60 \
-P 33@87:1920x1080+0+0@XR24 \
-P 39@87:200x200+100+600@AR24 \
-P 45@87:200x200+200+500@AR24 \
-P 51@87:200x200+300+400@AR24 \
-P 57@87:200x200+400+300@AR24 \
-P 63@87:200x200+500+200@AR24 \
-P 69@87:200x200+600+100@AR24 \
-P 81@87:200x200+700+000@AR24

Signed-off-by: Arnaud Vrac <[email protected]>
---
Arnaud Vrac (11):
drm/msm/dpu: tweak msm8998 hw catalog values
drm/msm/dpu: use the actual lm maximum width instead of a hardcoded value
drm/msm/dpu: use hsync/vsync polarity set by the encoder
drm/msm/dpu: allow using lm mixer base stage
drm/msm/dpu: allow using all lm mixer stages
drm/msm/dpu: support cursor sspp hw blocks
drm/msm/dpu: add sspp cursor blocks to msm8998 hw catalog
drm/msm/dpu: fix cursor block register bit offset in msm8998 hw catalog
drm/msm/dpu: set max cursor width to 512x512
drm/msm/dpu: tweak lm pairings in msm8998 hw catalog
drm/msm/dpu: do not use mixer that supports dspp when not required

.../drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h | 28 +++++++------
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 10 ++---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 49 +++++++++++++++++-----
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 16 ++-----
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 22 +++++++---
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 32 +++++++++++---
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 2 +-
7 files changed, 107 insertions(+), 52 deletions(-)
---
base-commit: e3342532ecd39bbd9c2ab5b9001cec1589bc37e9
change-id: 20230419-dpu-tweaks-5475305621d9

Best regards,
--
Arnaud Vrac <[email protected]>


2023-04-19 14:42:30

by Arnaud Vrac

[permalink] [raw]
Subject: [PATCH 01/11] drm/msm/dpu: tweak msm8998 hw catalog values

Match the values found in the downstream msm-4.4 kernel sde driver.

Signed-off-by: Arnaud Vrac <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h | 8 ++++----
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 15 +++++----------
2 files changed, 9 insertions(+), 14 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 2b3ae84057dfe..b07e8a9941f79 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
@@ -134,10 +134,10 @@ static const struct dpu_dspp_cfg msm8998_dspp[] = {
};

static const struct dpu_intf_cfg msm8998_intf[] = {
- INTF_BLK("intf_0", INTF_0, 0x6a000, 0x280, INTF_DP, 0, 25, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
- INTF_BLK("intf_1", INTF_1, 0x6a800, 0x280, INTF_DSI, 0, 25, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
- INTF_BLK("intf_2", INTF_2, 0x6b000, 0x280, INTF_DSI, 1, 25, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 28, 29),
- INTF_BLK("intf_3", INTF_3, 0x6b800, 0x280, INTF_HDMI, 0, 25, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 30, 31),
+ INTF_BLK("intf_0", INTF_0, 0x6a000, 0x280, INTF_DP, 0, 21, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
+ INTF_BLK("intf_1", INTF_1, 0x6a800, 0x280, INTF_DSI, 0, 21, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
+ INTF_BLK("intf_2", INTF_2, 0x6b000, 0x280, INTF_DSI, 1, 21, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 28, 29),
+ INTF_BLK("intf_3", INTF_3, 0x6b800, 0x280, INTF_HDMI, 0, 21, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 30, 31),
};

static const struct dpu_perf_cfg msm8998_perf_data = {
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 03f162af1a50b..8d5d782a43398 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -587,12 +587,12 @@ static const u32 sdm845_nrt_pri_lvl[] = {3, 3, 3, 3, 3, 3, 3, 3};

static const struct dpu_vbif_dynamic_ot_cfg msm8998_ot_rdwr_cfg[] = {
{
- .pps = 1088 * 1920 * 30,
+ .pps = 1920 * 1080 * 30,
.ot_limit = 2,
},
{
- .pps = 1088 * 1920 * 60,
- .ot_limit = 6,
+ .pps = 1920 * 1080 * 60,
+ .ot_limit = 4,
},
{
.pps = 3840 * 2160 * 30,
@@ -705,10 +705,7 @@ static const struct dpu_qos_lut_entry msm8998_qos_linear[] = {
{.fl = 10, .lut = 0x1555b},
{.fl = 11, .lut = 0x5555b},
{.fl = 12, .lut = 0x15555b},
- {.fl = 13, .lut = 0x55555b},
- {.fl = 14, .lut = 0},
- {.fl = 1, .lut = 0x1b},
- {.fl = 0, .lut = 0}
+ {.fl = 0, .lut = 0x55555b}
};

static const struct dpu_qos_lut_entry sdm845_qos_linear[] = {
@@ -730,9 +727,7 @@ static const struct dpu_qos_lut_entry msm8998_qos_macrotile[] = {
{.fl = 10, .lut = 0x1aaff},
{.fl = 11, .lut = 0x5aaff},
{.fl = 12, .lut = 0x15aaff},
- {.fl = 13, .lut = 0x55aaff},
- {.fl = 1, .lut = 0x1aaff},
- {.fl = 0, .lut = 0},
+ {.fl = 0, .lut = 0x55aaff},
};

static const struct dpu_qos_lut_entry sc7180_qos_linear[] = {

--
2.40.0

2023-04-19 14:42:37

by Arnaud Vrac

[permalink] [raw]
Subject: [PATCH 03/11] drm/msm/dpu: use hsync/vsync polarity set by the encoder

Do not override the hsync/vsync polarity passed by the encoder when
setting up intf timings. The same logic was used in both the encoder and
intf code to set the DP and DSI polarities, so those interfaces are not
impacted. However for HDMI, the polarities were overriden to static
values based on the vertical resolution, instead of using the actual
mode polarities.

Signed-off-by: Arnaud Vrac <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 16 +++-------------
1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
index 84ee2efa9c664..9f05417eb1213 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -104,7 +104,7 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
u32 active_h_start, active_h_end;
u32 active_v_start, active_v_end;
u32 active_hctl, display_hctl, hsync_ctl;
- u32 polarity_ctl, den_polarity, hsync_polarity, vsync_polarity;
+ u32 polarity_ctl, den_polarity;
u32 panel_format;
u32 intf_cfg, intf_cfg2 = 0;
u32 display_data_hctl = 0, active_data_hctl = 0;
@@ -191,19 +191,9 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
}

den_polarity = 0;
- if (ctx->cap->type == INTF_HDMI) {
- hsync_polarity = p->yres >= 720 ? 0 : 1;
- vsync_polarity = p->yres >= 720 ? 0 : 1;
- } else if (ctx->cap->type == INTF_DP) {
- hsync_polarity = p->hsync_polarity;
- vsync_polarity = p->vsync_polarity;
- } else {
- hsync_polarity = 0;
- vsync_polarity = 0;
- }
polarity_ctl = (den_polarity << 2) | /* DEN Polarity */
- (vsync_polarity << 1) | /* VSYNC Polarity */
- (hsync_polarity << 0); /* HSYNC Polarity */
+ (p->vsync_polarity << 1) | /* VSYNC Polarity */
+ (p->hsync_polarity << 0); /* HSYNC Polarity */

if (!DPU_FORMAT_IS_YUV(fmt))
panel_format = (fmt->bits[C0_G_Y] |

--
2.40.0

2023-04-19 14:42:41

by Arnaud Vrac

[permalink] [raw]
Subject: [PATCH 04/11] drm/msm/dpu: allow using lm mixer base stage

The dpu backend already handles applying alpha to the base stage, so we
can use it to render the bottom plane in all cases. This allows mixing
one additional plane with the hardware mixer.

Signed-off-by: Arnaud Vrac <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 14b5cfe306113..148921ed62f85 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -881,7 +881,7 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
r_pipe->sspp = NULL;

- pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos;
+ pstate->stage = DPU_STAGE_BASE + pstate->base.normalized_zpos;
if (pstate->stage >= pdpu->catalog->caps->max_mixer_blendstages) {
DPU_ERROR("> %d plane stages assigned\n",
pdpu->catalog->caps->max_mixer_blendstages - DPU_STAGE_0);

--
2.40.0

2023-04-19 14:42:47

by Arnaud Vrac

[permalink] [raw]
Subject: [PATCH 02/11] drm/msm/dpu: use the actual lm maximum width instead of a hardcoded value

This avoids using two LMs instead of one when the display width is lower
than the maximum supported value. For example on MSM8996/MSM8998, the
actual maxwidth is 2560, so we would use two LMs for 1280x720 or
1920x1080 resolutions, while one is enough.

Signed-off-by: Arnaud Vrac <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 1dc5dbe585723..dd2914726c4f6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -53,8 +53,6 @@

#define IDLE_SHORT_TIMEOUT 1

-#define MAX_HDISPLAY_SPLIT 1080
-
/* timeout in frames waiting for frame done */
#define DPU_ENCODER_FRAME_DONE_TIMEOUT_FRAMES 5

@@ -568,10 +566,12 @@ static struct msm_display_topology dpu_encoder_get_topology(
*/
if (intf_count == 2)
topology.num_lm = 2;
- else if (!dpu_kms->catalog->caps->has_3d_merge)
- topology.num_lm = 1;
+ else if (dpu_kms->catalog->caps->has_3d_merge &&
+ dpu_kms->catalog->mixer_count > 0 &&
+ mode->hdisplay > dpu_kms->catalog->mixer[0].sblk->maxwidth)
+ topology.num_lm = 2;
else
- topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 : 1;
+ topology.num_lm = 1;

if (crtc_state->ctm)
topology.num_dspp = topology.num_lm;

--
2.40.0

2023-04-19 14:42:48

by Arnaud Vrac

[permalink] [raw]
Subject: [PATCH 05/11] drm/msm/dpu: allow using all lm mixer stages

The max_mixer_blendstages hw catalog property represents the number of
planes that can be blended by the lm mixer, excluding the base stage, so
adjust the check for the number of currently assigned planes accordingly.

Signed-off-by: Arnaud Vrac <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 148921ed62f85..128ecdc145260 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -882,9 +882,9 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
r_pipe->sspp = NULL;

pstate->stage = DPU_STAGE_BASE + pstate->base.normalized_zpos;
- if (pstate->stage >= pdpu->catalog->caps->max_mixer_blendstages) {
- DPU_ERROR("> %d plane stages assigned\n",
- pdpu->catalog->caps->max_mixer_blendstages - DPU_STAGE_0);
+ if (pstate->stage > DPU_STAGE_BASE + pdpu->catalog->caps->max_mixer_blendstages) {
+ DPU_ERROR("> %d plane mixer stages assigned\n",
+ pdpu->catalog->caps->max_mixer_blendstages);
return -EINVAL;
}


--
2.40.0

2023-04-19 14:42:50

by Arnaud Vrac

[permalink] [raw]
Subject: [PATCH 06/11] drm/msm/dpu: support cursor sspp hw blocks

Cursor SSPP must be assigned to the last mixer stage, so we assign an
immutable zpos property with a value higher than primary/overlay planes,
to ensure it will always be on top.

Signed-off-by: Arnaud Vrac <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 19 ++++++++++++++-----
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 26 +++++++++++++++++++++++---
2 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 0e7a68714e9e1..6cce0f6cfcb01 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -738,13 +738,22 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
for (i = 0; i < catalog->sspp_count; i++) {
enum drm_plane_type type;

- if ((catalog->sspp[i].features & BIT(DPU_SSPP_CURSOR))
- && cursor_planes_idx < max_crtc_count)
- type = DRM_PLANE_TYPE_CURSOR;
- else if (primary_planes_idx < max_crtc_count)
+ if (catalog->sspp[i].features & BIT(DPU_SSPP_CURSOR)) {
+ if (cursor_planes_idx < max_crtc_count) {
+ type = DRM_PLANE_TYPE_CURSOR;
+ } else if (catalog->sspp[i].type == SSPP_TYPE_CURSOR) {
+ /* Cursor SSPP can only be used in the last
+ * mixer stage, so it doesn't make sense to
+ * assign two of those to the same CRTC */
+ continue;
+ } else {
+ type = DRM_PLANE_TYPE_OVERLAY;
+ }
+ } else if (primary_planes_idx < max_crtc_count) {
type = DRM_PLANE_TYPE_PRIMARY;
- else
+ } else {
type = DRM_PLANE_TYPE_OVERLAY;
+ }

DPU_DEBUG("Create plane type %d with features %lx (cur %lx)\n",
type, catalog->sspp[i].features,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 128ecdc145260..5a7bb8543866c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -881,7 +881,14 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
r_pipe->sspp = NULL;

- pstate->stage = DPU_STAGE_BASE + pstate->base.normalized_zpos;
+ if (pipe_hw_caps->type == SSPP_TYPE_CURSOR) {
+ /* enforce cursor sspp to use the last mixer stage */
+ pstate->stage = DPU_STAGE_BASE +
+ pdpu->catalog->caps->max_mixer_blendstages;
+ } else {
+ pstate->stage = DPU_STAGE_BASE + pstate->base.normalized_zpos;
+ }
+
if (pstate->stage > DPU_STAGE_BASE + pdpu->catalog->caps->max_mixer_blendstages) {
DPU_ERROR("> %d plane mixer stages assigned\n",
pdpu->catalog->caps->max_mixer_blendstages);
@@ -1463,6 +1470,7 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev,
struct msm_drm_private *priv = dev->dev_private;
struct dpu_kms *kms = to_dpu_kms(priv->kms);
struct dpu_hw_sspp *pipe_hw;
+ const uint64_t *format_modifiers;
uint32_t num_formats;
uint32_t supported_rotations;
int ret = -EINVAL;
@@ -1489,15 +1497,27 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev,
format_list = pipe_hw->cap->sblk->format_list;
num_formats = pipe_hw->cap->sblk->num_formats;

+ if (pipe_hw->cap->type == SSPP_TYPE_CURSOR)
+ format_modifiers = NULL;
+ else
+ format_modifiers = supported_format_modifiers;
+
ret = drm_universal_plane_init(dev, plane, 0xff, &dpu_plane_funcs,
format_list, num_formats,
- supported_format_modifiers, type, NULL);
+ format_modifiers, type, NULL);
if (ret)
goto clean_plane;

pdpu->catalog = kms->catalog;

- ret = drm_plane_create_zpos_property(plane, 0, 0, DPU_ZPOS_MAX);
+ if (pipe_hw->cap->type == SSPP_TYPE_CURSOR) {
+ /* cursor SSPP can only be used in the last mixer stage,
+ * enforce it by maxing out the cursor plane zpos */
+ ret = drm_plane_create_zpos_immutable_property(plane, DPU_ZPOS_MAX);
+ } else {
+ ret = drm_plane_create_zpos_property(plane, 0, 0, DPU_ZPOS_MAX - 1);
+ }
+
if (ret)
DPU_ERROR("failed to install zpos property, rc = %d\n", ret);


--
2.40.0

2023-04-19 14:42:52

by Arnaud Vrac

[permalink] [raw]
Subject: [PATCH 07/11] drm/msm/dpu: add sspp cursor blocks to msm8998 hw catalog

Now that cursor sspp blocks can be used for cursor planes, enable them
on msm8998. The dma sspp blocks that were assigned to cursor planes can
now be used for overlay planes instead.

Signed-off-by: Arnaud Vrac <[email protected]>
---
.../drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h | 8 +++--
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 34 ++++++++++++++++++++++
2 files changed, 40 insertions(+), 2 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 b07e8a9941f79..7de393b0f91d7 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
@@ -90,10 +90,14 @@ static const struct dpu_sspp_cfg msm8998_sspp[] = {
sdm845_dma_sblk_0, 1, SSPP_TYPE_DMA, DPU_CLK_CTRL_DMA0),
SSPP_BLK("sspp_9", SSPP_DMA1, 0x26000, 0x1ac, DMA_MSM8998_MASK,
sdm845_dma_sblk_1, 5, SSPP_TYPE_DMA, DPU_CLK_CTRL_DMA1),
- SSPP_BLK("sspp_10", SSPP_DMA2, 0x28000, 0x1ac, DMA_CURSOR_MSM8998_MASK,
+ SSPP_BLK("sspp_10", SSPP_DMA2, 0x28000, 0x1ac, DMA_MSM8998_MASK,
sdm845_dma_sblk_2, 9, SSPP_TYPE_DMA, DPU_CLK_CTRL_DMA2),
- SSPP_BLK("sspp_11", SSPP_DMA3, 0x2a000, 0x1ac, DMA_CURSOR_MSM8998_MASK,
+ SSPP_BLK("sspp_11", SSPP_DMA3, 0x2a000, 0x1ac, DMA_MSM8998_MASK,
sdm845_dma_sblk_3, 13, SSPP_TYPE_DMA, DPU_CLK_CTRL_DMA3),
+ SSPP_BLK("sspp_12", SSPP_CURSOR0, 0x34000, 0x1ac, DMA_CURSOR_MSM8998_MASK,
+ msm8998_cursor_sblk_0, 2, SSPP_TYPE_CURSOR, DPU_CLK_CTRL_CURSOR0),
+ SSPP_BLK("sspp_13", SSPP_CURSOR1, 0x36000, 0x1ac, DMA_CURSOR_MSM8998_MASK,
+ msm8998_cursor_sblk_1, 10, SSPP_TYPE_CURSOR, DPU_CLK_CTRL_CURSOR1),
};

static const struct dpu_lm_cfg msm8998_lm[] = {
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 8d5d782a43398..f34fa704936bc 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -242,6 +242,22 @@ static const uint32_t wb2_formats[] = {
DRM_FORMAT_XBGR4444,
};

+static const uint32_t cursor_formats[] = {
+ DRM_FORMAT_ARGB8888,
+ DRM_FORMAT_ABGR8888,
+ DRM_FORMAT_RGBA8888,
+ DRM_FORMAT_BGRA8888,
+ DRM_FORMAT_XRGB8888,
+ DRM_FORMAT_ARGB1555,
+ DRM_FORMAT_ABGR1555,
+ DRM_FORMAT_RGBA5551,
+ DRM_FORMAT_BGRA5551,
+ DRM_FORMAT_ARGB4444,
+ DRM_FORMAT_ABGR4444,
+ DRM_FORMAT_RGBA4444,
+ DRM_FORMAT_BGRA4444,
+};
+
/*************************************************************
* SSPP sub blocks config
*************************************************************/
@@ -300,6 +316,19 @@ static const uint32_t wb2_formats[] = {
.virt_num_formats = ARRAY_SIZE(plane_formats), \
}

+#define _CURSOR_SBLK(num) \
+ { \
+ .maxdwnscale = SSPP_UNITY_SCALE, \
+ .maxupscale = SSPP_UNITY_SCALE, \
+ .smart_dma_priority = 0, \
+ .src_blk = {.name = STRCAT("sspp_src_", num), \
+ .id = DPU_SSPP_SRC, .base = 0x00, .len = 0x150,}, \
+ .format_list = cursor_formats, \
+ .num_formats = ARRAY_SIZE(cursor_formats), \
+ .virt_format_list = cursor_formats, \
+ .virt_num_formats = ARRAY_SIZE(cursor_formats), \
+ }
+
static const struct dpu_sspp_sub_blks msm8998_vig_sblk_0 =
_VIG_SBLK("0", 0, DPU_SSPP_SCALER_QSEED3);
static const struct dpu_sspp_sub_blks msm8998_vig_sblk_1 =
@@ -309,6 +338,11 @@ static const struct dpu_sspp_sub_blks msm8998_vig_sblk_2 =
static const struct dpu_sspp_sub_blks msm8998_vig_sblk_3 =
_VIG_SBLK("3", 0, DPU_SSPP_SCALER_QSEED3);

+static const struct dpu_sspp_sub_blks msm8998_cursor_sblk_0 =
+ _CURSOR_SBLK("12");
+static const struct dpu_sspp_sub_blks msm8998_cursor_sblk_1 =
+ _CURSOR_SBLK("13");
+
static const struct dpu_rotation_cfg dpu_rot_sc7280_cfg_v2 = {
.rot_maxheight = 1088,
.rot_num_formats = ARRAY_SIZE(rotation_v2_formats),

--
2.40.0

2023-04-19 14:44:31

by Arnaud Vrac

[permalink] [raw]
Subject: [PATCH 08/11] drm/msm/dpu: fix cursor block register bit offset in msm8998 hw catalog

This matches the value for both fbdev and sde implementations in the
downstream msm-4.4 repository.

Signed-off-by: Arnaud Vrac <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h | 4 ++--
1 file changed, 2 insertions(+), 2 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 7de393b0f91d7..5ae1d41e3fa92 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
@@ -39,8 +39,8 @@ static const struct dpu_mdp_cfg msm8998_mdp[] = {
.clk_ctrls[DPU_CLK_CTRL_DMA1] = { .reg_off = 0x2b4, .bit_off = 8 },
.clk_ctrls[DPU_CLK_CTRL_DMA2] = { .reg_off = 0x2c4, .bit_off = 8 },
.clk_ctrls[DPU_CLK_CTRL_DMA3] = { .reg_off = 0x2c4, .bit_off = 12 },
- .clk_ctrls[DPU_CLK_CTRL_CURSOR0] = { .reg_off = 0x3a8, .bit_off = 15 },
- .clk_ctrls[DPU_CLK_CTRL_CURSOR1] = { .reg_off = 0x3b0, .bit_off = 15 },
+ .clk_ctrls[DPU_CLK_CTRL_CURSOR0] = { .reg_off = 0x3a8, .bit_off = 16 },
+ .clk_ctrls[DPU_CLK_CTRL_CURSOR1] = { .reg_off = 0x3b0, .bit_off = 16 },
},
};


--
2.40.0

2023-04-19 14:52:57

by Arnaud Vrac

[permalink] [raw]
Subject: [PATCH 10/11] drm/msm/dpu: tweak lm pairings in msm8998 hw catalog

Change lm blocks pairs so that lm blocks with the same features are
paired together:

LM_0 and LM_1 with PP and DSPP
LM_2 and LM_5 with PP
LM_3 and LM_4

This matches the sdm845 configuration and allows using pp or dspp when 2
lm blocks are needed in the topology. In the previous config the
reservation code could never find an lm pair without a matching feature
set.

Signed-off-by: Arnaud Vrac <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h | 8 ++++----
1 file changed, 4 insertions(+), 4 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 5ae1d41e3fa92..90db622eff4fa 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
@@ -102,17 +102,17 @@ static const struct dpu_sspp_cfg msm8998_sspp[] = {

static const struct dpu_lm_cfg msm8998_lm[] = {
LM_BLK("lm_0", LM_0, 0x44000, MIXER_MSM8998_MASK,
- &msm8998_lm_sblk, PINGPONG_0, LM_2, DSPP_0),
+ &msm8998_lm_sblk, PINGPONG_0, LM_1, DSPP_0),
LM_BLK("lm_1", LM_1, 0x45000, MIXER_MSM8998_MASK,
- &msm8998_lm_sblk, PINGPONG_1, LM_5, DSPP_1),
+ &msm8998_lm_sblk, PINGPONG_1, LM_0, DSPP_1),
LM_BLK("lm_2", LM_2, 0x46000, MIXER_MSM8998_MASK,
- &msm8998_lm_sblk, PINGPONG_2, LM_0, 0),
+ &msm8998_lm_sblk, PINGPONG_2, LM_5, 0),
LM_BLK("lm_3", LM_3, 0x47000, MIXER_MSM8998_MASK,
&msm8998_lm_sblk, PINGPONG_MAX, 0, 0),
LM_BLK("lm_4", LM_4, 0x48000, MIXER_MSM8998_MASK,
&msm8998_lm_sblk, PINGPONG_MAX, 0, 0),
LM_BLK("lm_5", LM_5, 0x49000, MIXER_MSM8998_MASK,
- &msm8998_lm_sblk, PINGPONG_3, LM_1, 0),
+ &msm8998_lm_sblk, PINGPONG_3, LM_2, 0),
};

static const struct dpu_pingpong_cfg msm8998_pp[] = {

--
2.40.0

2023-04-19 14:56:51

by Arnaud Vrac

[permalink] [raw]
Subject: [PATCH 11/11] drm/msm/dpu: do not use mixer that supports dspp when not required

This avoids using lm blocks that support DSPP when not needed, to
keep those resources available.

Signed-off-by: Arnaud Vrac <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index f4dda88a73f7d..4b393d46c743f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -362,7 +362,7 @@ static bool _dpu_rm_check_lm_and_get_connected_blks(struct dpu_rm *rm,
*pp_idx = idx;

if (!reqs->topology.num_dspp)
- return true;
+ return !lm_cfg->dspp;

idx = lm_cfg->dspp - DSPP_0;
if (idx < 0 || idx >= ARRAY_SIZE(rm->dspp_blks)) {

--
2.40.0

2023-04-19 15:04:14

by Arnaud Vrac

[permalink] [raw]
Subject: [PATCH 09/11] drm/msm/dpu: set max cursor width to 512x512

Override the default max cursor size reported to userspace of 64x64.
MSM8998 hw cursor planes support 512x512 size, and other chips use DMA
SSPPs.

Signed-off-by: Arnaud Vrac <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 6cce0f6cfcb01..2dd19b7aca0f8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1014,6 +1014,9 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
dpu_kms = to_dpu_kms(kms);
dev = dpu_kms->dev;

+ dev->mode_config.cursor_width = 512;
+ dev->mode_config.cursor_height = 512;
+
rc = dpu_kms_global_obj_init(dpu_kms);
if (rc)
return rc;

--
2.40.0

2023-04-19 22:18:16

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [PATCH 01/11] drm/msm/dpu: tweak msm8998 hw catalog values

On 19/04/2023 17:41, Arnaud Vrac wrote:
> Match the values found in the downstream msm-4.4 kernel sde driver.
>
> Signed-off-by: Arnaud Vrac <[email protected]>

Fixes: 94391a14fc27 ("drm/msm/dpu1: Add MSM8998 to hw catalog")

Reviewed-by: Dmitry Baryshkov <[email protected]>

> ---
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h | 8 ++++----
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 15 +++++----------
> 2 files changed, 9 insertions(+), 14 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 2b3ae84057dfe..b07e8a9941f79 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
> @@ -134,10 +134,10 @@ static const struct dpu_dspp_cfg msm8998_dspp[] = {
> };
>
> static const struct dpu_intf_cfg msm8998_intf[] = {
> - INTF_BLK("intf_0", INTF_0, 0x6a000, 0x280, INTF_DP, 0, 25, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> - INTF_BLK("intf_1", INTF_1, 0x6a800, 0x280, INTF_DSI, 0, 25, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> - INTF_BLK("intf_2", INTF_2, 0x6b000, 0x280, INTF_DSI, 1, 25, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 28, 29),
> - INTF_BLK("intf_3", INTF_3, 0x6b800, 0x280, INTF_HDMI, 0, 25, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 30, 31),
> + INTF_BLK("intf_0", INTF_0, 0x6a000, 0x280, INTF_DP, 0, 21, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
> + INTF_BLK("intf_1", INTF_1, 0x6a800, 0x280, INTF_DSI, 0, 21, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
> + INTF_BLK("intf_2", INTF_2, 0x6b000, 0x280, INTF_DSI, 1, 21, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 28, 29),
> + INTF_BLK("intf_3", INTF_3, 0x6b800, 0x280, INTF_HDMI, 0, 21, INTF_SDM845_MASK, MDP_SSPP_TOP0_INTR, 30, 31),
> };
>
> static const struct dpu_perf_cfg msm8998_perf_data = {
> 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 03f162af1a50b..8d5d782a43398 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> @@ -587,12 +587,12 @@ static const u32 sdm845_nrt_pri_lvl[] = {3, 3, 3, 3, 3, 3, 3, 3};
>
> static const struct dpu_vbif_dynamic_ot_cfg msm8998_ot_rdwr_cfg[] = {
> {
> - .pps = 1088 * 1920 * 30,
> + .pps = 1920 * 1080 * 30,
> .ot_limit = 2,
> },
> {
> - .pps = 1088 * 1920 * 60,
> - .ot_limit = 6,
> + .pps = 1920 * 1080 * 60,
> + .ot_limit = 4,
> },
> {
> .pps = 3840 * 2160 * 30,
> @@ -705,10 +705,7 @@ static const struct dpu_qos_lut_entry msm8998_qos_linear[] = {
> {.fl = 10, .lut = 0x1555b},
> {.fl = 11, .lut = 0x5555b},
> {.fl = 12, .lut = 0x15555b},
> - {.fl = 13, .lut = 0x55555b},
> - {.fl = 14, .lut = 0},
> - {.fl = 1, .lut = 0x1b},
> - {.fl = 0, .lut = 0}
> + {.fl = 0, .lut = 0x55555b}
> };
>
> static const struct dpu_qos_lut_entry sdm845_qos_linear[] = {
> @@ -730,9 +727,7 @@ static const struct dpu_qos_lut_entry msm8998_qos_macrotile[] = {
> {.fl = 10, .lut = 0x1aaff},
> {.fl = 11, .lut = 0x5aaff},
> {.fl = 12, .lut = 0x15aaff},
> - {.fl = 13, .lut = 0x55aaff},
> - {.fl = 1, .lut = 0x1aaff},
> - {.fl = 0, .lut = 0},
> + {.fl = 0, .lut = 0x55aaff},
> };
>
> static const struct dpu_qos_lut_entry sc7180_qos_linear[] = {
>

--
With best wishes
Dmitry

2023-04-19 22:52:27

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [PATCH 03/11] drm/msm/dpu: use hsync/vsync polarity set by the encoder

On 19/04/2023 17:41, Arnaud Vrac wrote:
> Do not override the hsync/vsync polarity passed by the encoder when
> setting up intf timings. The same logic was used in both the encoder and
> intf code to set the DP and DSI polarities, so those interfaces are not
> impacted. However for HDMI, the polarities were overriden to static
> values based on the vertical resolution, instead of using the actual
> mode polarities.
>
> Signed-off-by: Arnaud Vrac <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 16 +++-------------
> 1 file changed, 3 insertions(+), 13 deletions(-)

Reviewed-by: Dmitry Baryshkov <[email protected]>

As a side note: I think at some point we should get rid of the override
in dpu_encoder too and move it to dsi bridge code.

--
With best wishes
Dmitry

2023-04-19 22:53:05

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [PATCH 04/11] drm/msm/dpu: allow using lm mixer base stage

On 19/04/2023 17:41, Arnaud Vrac wrote:
> The dpu backend already handles applying alpha to the base stage, so we
> can use it to render the bottom plane in all cases. This allows mixing
> one additional plane with the hardware mixer.
>
> Signed-off-by: Arnaud Vrac <[email protected]>

This might require additional changes. First, for the STAGE_BASE pipe
in the source split mode (iow using two LMs) should programmed with
respect to the right LM's x offset (rather than usual left top-left LM).
See mdss_mdp_pipe_position_update().

Also this might need some interaction with CTL_MIXER_BORDER_OUT being
set or not. If I remember correctly, if there bottom plane is not
fullscreen or if there are no planes at all, we should set
CTL_MIXER_BORDER_OUT (which takes STAGE_BASE) and start assigning them
from STAGE0. If not, we can use STAGE_BASE.

> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index 14b5cfe306113..148921ed62f85 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -881,7 +881,7 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
> r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> r_pipe->sspp = NULL;
>
> - pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos;
> + pstate->stage = DPU_STAGE_BASE + pstate->base.normalized_zpos;
> if (pstate->stage >= pdpu->catalog->caps->max_mixer_blendstages) {
> DPU_ERROR("> %d plane stages assigned\n",
> pdpu->catalog->caps->max_mixer_blendstages - DPU_STAGE_0);
>

--
With best wishes
Dmitry

2023-04-19 22:54:29

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [PATCH 05/11] drm/msm/dpu: allow using all lm mixer stages

On 19/04/2023 17:41, Arnaud Vrac wrote:
> The max_mixer_blendstages hw catalog property represents the number of
> planes that can be blended by the lm mixer, excluding the base stage, so
> adjust the check for the number of currently assigned planes accordingly.
>
> Signed-off-by: Arnaud Vrac <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Dmitry Baryshkov <[email protected]>

--
With best wishes
Dmitry

2023-04-19 23:00:19

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [PATCH 06/11] drm/msm/dpu: support cursor sspp hw blocks

On 19/04/2023 17:41, Arnaud Vrac wrote:
> Cursor SSPP must be assigned to the last mixer stage, so we assign an
> immutable zpos property with a value higher than primary/overlay planes,
> to ensure it will always be on top.

The commit does more than is described in the commit message. Let's do
it step by step. Please split into several patches. Also see below.

>
> Signed-off-by: Arnaud Vrac <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 19 ++++++++++++++-----
> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 26 +++++++++++++++++++++++---
> 2 files changed, 37 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 0e7a68714e9e1..6cce0f6cfcb01 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -738,13 +738,22 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
> for (i = 0; i < catalog->sspp_count; i++) {
> enum drm_plane_type type;
>
> - if ((catalog->sspp[i].features & BIT(DPU_SSPP_CURSOR))
> - && cursor_planes_idx < max_crtc_count)
> - type = DRM_PLANE_TYPE_CURSOR;
> - else if (primary_planes_idx < max_crtc_count)
> + if (catalog->sspp[i].features & BIT(DPU_SSPP_CURSOR)) {
> + if (cursor_planes_idx < max_crtc_count) {
> + type = DRM_PLANE_TYPE_CURSOR;
> + } else if (catalog->sspp[i].type == SSPP_TYPE_CURSOR) {
> + /* Cursor SSPP can only be used in the last
> + * mixer stage, so it doesn't make sense to
> + * assign two of those to the same CRTC */
> + continue;
> + } else {
> + type = DRM_PLANE_TYPE_OVERLAY;
> + }
> + } else if (primary_planes_idx < max_crtc_count) {
> type = DRM_PLANE_TYPE_PRIMARY;
> - else
> + } else {
> type = DRM_PLANE_TYPE_OVERLAY;
> + }

Ack. I'm not sure how compositors will cope if we have two planes with
immutable zpos set to the same value. Also I'd prefer to have this as a
separate commit.

>
> DPU_DEBUG("Create plane type %d with features %lx (cur %lx)\n",
> type, catalog->sspp[i].features,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index 128ecdc145260..5a7bb8543866c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -881,7 +881,14 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
> r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> r_pipe->sspp = NULL;
>
> - pstate->stage = DPU_STAGE_BASE + pstate->base.normalized_zpos;
> + if (pipe_hw_caps->type == SSPP_TYPE_CURSOR) {
> + /* enforce cursor sspp to use the last mixer stage */

I'd add here 'we know that it is the last plane in the stack because of
zpos property ranges'

> + pstate->stage = DPU_STAGE_BASE +
> + pdpu->catalog->caps->max_mixer_blendstages;
> + } else {
> + pstate->stage = DPU_STAGE_BASE + pstate->base.normalized_zpos;
> + }
> +
> if (pstate->stage > DPU_STAGE_BASE + pdpu->catalog->caps->max_mixer_blendstages) {
> DPU_ERROR("> %d plane mixer stages assigned\n",
> pdpu->catalog->caps->max_mixer_blendstages);
> @@ -1463,6 +1470,7 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev,
> struct msm_drm_private *priv = dev->dev_private;
> struct dpu_kms *kms = to_dpu_kms(priv->kms);
> struct dpu_hw_sspp *pipe_hw;
> + const uint64_t *format_modifiers;
> uint32_t num_formats;
> uint32_t supported_rotations;
> int ret = -EINVAL;
> @@ -1489,15 +1497,27 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev,
> format_list = pipe_hw->cap->sblk->format_list;
> num_formats = pipe_hw->cap->sblk->num_formats;
>
> + if (pipe_hw->cap->type == SSPP_TYPE_CURSOR)
> + format_modifiers = NULL;
> + else
> + format_modifiers = supported_format_modifiers;
> +
> ret = drm_universal_plane_init(dev, plane, 0xff, &dpu_plane_funcs,
> format_list, num_formats,
> - supported_format_modifiers, type, NULL);
> + format_modifiers, type, NULL);


Separate commit please

> if (ret)
> goto clean_plane;
>
> pdpu->catalog = kms->catalog;
>
> - ret = drm_plane_create_zpos_property(plane, 0, 0, DPU_ZPOS_MAX);
> + if (pipe_hw->cap->type == SSPP_TYPE_CURSOR) {
> + /* cursor SSPP can only be used in the last mixer stage,
> + * enforce it by maxing out the cursor plane zpos */
> + ret = drm_plane_create_zpos_immutable_property(plane, DPU_ZPOS_MAX);
> + } else {
> + ret = drm_plane_create_zpos_property(plane, 0, 0, DPU_ZPOS_MAX - 1);
> + }
> +
> if (ret)
> DPU_ERROR("failed to install zpos property, rc = %d\n", ret);
>
>

--
With best wishes
Dmitry

2023-04-19 23:01:12

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [PATCH 02/11] drm/msm/dpu: use the actual lm maximum width instead of a hardcoded value

On 19/04/2023 17:41, Arnaud Vrac wrote:
> This avoids using two LMs instead of one when the display width is lower
> than the maximum supported value. For example on MSM8996/MSM8998, the
> actual maxwidth is 2560, so we would use two LMs for 1280x720 or
> 1920x1080 resolutions, while one is enough.
>
> Signed-off-by: Arnaud Vrac <[email protected]>

While this looks correct (and following what we have in 4.4), later
vendor kernels specify the topology explicitly. Probably we should check
this with the hw guys, because it might be the following case: even
though a single LM can supply the mode, it will spend more power
compared to two LMs.


> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 1dc5dbe585723..dd2914726c4f6 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -53,8 +53,6 @@
>
> #define IDLE_SHORT_TIMEOUT 1
>
> -#define MAX_HDISPLAY_SPLIT 1080
> -
> /* timeout in frames waiting for frame done */
> #define DPU_ENCODER_FRAME_DONE_TIMEOUT_FRAMES 5
>
> @@ -568,10 +566,12 @@ static struct msm_display_topology dpu_encoder_get_topology(
> */
> if (intf_count == 2)
> topology.num_lm = 2;
> - else if (!dpu_kms->catalog->caps->has_3d_merge)
> - topology.num_lm = 1;
> + else if (dpu_kms->catalog->caps->has_3d_merge &&
> + dpu_kms->catalog->mixer_count > 0 &&
> + mode->hdisplay > dpu_kms->catalog->mixer[0].sblk->maxwidth)
> + topology.num_lm = 2;
> else
> - topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 : 1;
> + topology.num_lm = 1;
>
> if (crtc_state->ctm)
> topology.num_dspp = topology.num_lm;
>

--
With best wishes
Dmitry

2023-04-19 23:01:24

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [PATCH 09/11] drm/msm/dpu: set max cursor width to 512x512

On 19/04/2023 17:41, Arnaud Vrac wrote:
> Override the default max cursor size reported to userspace of 64x64.
> MSM8998 hw cursor planes support 512x512 size, and other chips use DMA
> SSPPs.
>
> Signed-off-by: Arnaud Vrac <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 3 +++
> 1 file changed, 3 insertions(+)

Reviewed-by: Dmitry Baryshkov <[email protected]>

--
With best wishes
Dmitry

2023-04-19 23:32:44

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [PATCH 07/11] drm/msm/dpu: add sspp cursor blocks to msm8998 hw catalog

On 19/04/2023 17:41, Arnaud Vrac wrote:
> Now that cursor sspp blocks can be used for cursor planes, enable them
> on msm8998. The dma sspp blocks that were assigned to cursor planes can
> now be used for overlay planes instead.

While the change is correct, there is more about it. Composers, using
universal planes, will see this plane too. They have no obligations to
use it only for the cursor. At the minimum could you please extend the
plane_atomic_check to check for the plane dimensions for the CURSOR pipes?

For this change:

Reviewed-by: Dmitry Baryshkov <[email protected]>

>
> Signed-off-by: Arnaud Vrac <[email protected]>
> ---
> .../drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h | 8 +++--
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 34 ++++++++++++++++++++++
> 2 files changed, 40 insertions(+), 2 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 b07e8a9941f79..7de393b0f91d7 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
> @@ -90,10 +90,14 @@ static const struct dpu_sspp_cfg msm8998_sspp[] = {
> sdm845_dma_sblk_0, 1, SSPP_TYPE_DMA, DPU_CLK_CTRL_DMA0),
> SSPP_BLK("sspp_9", SSPP_DMA1, 0x26000, 0x1ac, DMA_MSM8998_MASK,
> sdm845_dma_sblk_1, 5, SSPP_TYPE_DMA, DPU_CLK_CTRL_DMA1),
> - SSPP_BLK("sspp_10", SSPP_DMA2, 0x28000, 0x1ac, DMA_CURSOR_MSM8998_MASK,
> + SSPP_BLK("sspp_10", SSPP_DMA2, 0x28000, 0x1ac, DMA_MSM8998_MASK,
> sdm845_dma_sblk_2, 9, SSPP_TYPE_DMA, DPU_CLK_CTRL_DMA2),
> - SSPP_BLK("sspp_11", SSPP_DMA3, 0x2a000, 0x1ac, DMA_CURSOR_MSM8998_MASK,
> + SSPP_BLK("sspp_11", SSPP_DMA3, 0x2a000, 0x1ac, DMA_MSM8998_MASK,
> sdm845_dma_sblk_3, 13, SSPP_TYPE_DMA, DPU_CLK_CTRL_DMA3),
> + SSPP_BLK("sspp_12", SSPP_CURSOR0, 0x34000, 0x1ac, DMA_CURSOR_MSM8998_MASK,
> + msm8998_cursor_sblk_0, 2, SSPP_TYPE_CURSOR, DPU_CLK_CTRL_CURSOR0),
> + SSPP_BLK("sspp_13", SSPP_CURSOR1, 0x36000, 0x1ac, DMA_CURSOR_MSM8998_MASK,
> + msm8998_cursor_sblk_1, 10, SSPP_TYPE_CURSOR, DPU_CLK_CTRL_CURSOR1),
> };
>
> static const struct dpu_lm_cfg msm8998_lm[] = {
> 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 8d5d782a43398..f34fa704936bc 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> @@ -242,6 +242,22 @@ static const uint32_t wb2_formats[] = {
> DRM_FORMAT_XBGR4444,
> };
>
> +static const uint32_t cursor_formats[] = {
> + DRM_FORMAT_ARGB8888,
> + DRM_FORMAT_ABGR8888,
> + DRM_FORMAT_RGBA8888,
> + DRM_FORMAT_BGRA8888,
> + DRM_FORMAT_XRGB8888,
> + DRM_FORMAT_ARGB1555,
> + DRM_FORMAT_ABGR1555,
> + DRM_FORMAT_RGBA5551,
> + DRM_FORMAT_BGRA5551,
> + DRM_FORMAT_ARGB4444,
> + DRM_FORMAT_ABGR4444,
> + DRM_FORMAT_RGBA4444,
> + DRM_FORMAT_BGRA4444,
> +};
> +
> /*************************************************************
> * SSPP sub blocks config
> *************************************************************/
> @@ -300,6 +316,19 @@ static const uint32_t wb2_formats[] = {
> .virt_num_formats = ARRAY_SIZE(plane_formats), \
> }
>
> +#define _CURSOR_SBLK(num) \
> + { \
> + .maxdwnscale = SSPP_UNITY_SCALE, \
> + .maxupscale = SSPP_UNITY_SCALE, \
> + .smart_dma_priority = 0, \
> + .src_blk = {.name = STRCAT("sspp_src_", num), \
> + .id = DPU_SSPP_SRC, .base = 0x00, .len = 0x150,}, \
> + .format_list = cursor_formats, \
> + .num_formats = ARRAY_SIZE(cursor_formats), \
> + .virt_format_list = cursor_formats, \
> + .virt_num_formats = ARRAY_SIZE(cursor_formats), \
> + }
> +
> static const struct dpu_sspp_sub_blks msm8998_vig_sblk_0 =
> _VIG_SBLK("0", 0, DPU_SSPP_SCALER_QSEED3);
> static const struct dpu_sspp_sub_blks msm8998_vig_sblk_1 =
> @@ -309,6 +338,11 @@ static const struct dpu_sspp_sub_blks msm8998_vig_sblk_2 =
> static const struct dpu_sspp_sub_blks msm8998_vig_sblk_3 =
> _VIG_SBLK("3", 0, DPU_SSPP_SCALER_QSEED3);
>
> +static const struct dpu_sspp_sub_blks msm8998_cursor_sblk_0 =
> + _CURSOR_SBLK("12");
> +static const struct dpu_sspp_sub_blks msm8998_cursor_sblk_1 =
> + _CURSOR_SBLK("13");
> +
> static const struct dpu_rotation_cfg dpu_rot_sc7280_cfg_v2 = {
> .rot_maxheight = 1088,
> .rot_num_formats = ARRAY_SIZE(rotation_v2_formats),
>

--
With best wishes
Dmitry

2023-04-19 23:32:56

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [PATCH 08/11] drm/msm/dpu: fix cursor block register bit offset in msm8998 hw catalog

On 19/04/2023 17:41, Arnaud Vrac wrote:
> This matches the value for both fbdev and sde implementations in the
> downstream msm-4.4 repository.
>
> Signed-off-by: Arnaud Vrac <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Dmitry Baryshkov <[email protected]>

--
With best wishes
Dmitry

2023-04-19 23:33:50

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [PATCH 10/11] drm/msm/dpu: tweak lm pairings in msm8998 hw catalog

On 19/04/2023 17:41, Arnaud Vrac wrote:
> Change lm blocks pairs so that lm blocks with the same features are
> paired together:
>
> LM_0 and LM_1 with PP and DSPP
> LM_2 and LM_5 with PP
> LM_3 and LM_4
>
> This matches the sdm845 configuration and allows using pp or dspp when 2
> lm blocks are needed in the topology. In the previous config the
> reservation code could never find an lm pair without a matching feature
> set.

And this matches the hardcoded configuration in msm-4.4

Fixes: 94391a14fc27 ("drm/msm/dpu1: Add MSM8998 to hw catalog")

Reviewed-by: Dmitry Baryshkov <[email protected]>

>
> Signed-off-by: Arnaud Vrac <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)


--
With best wishes
Dmitry

2023-04-19 23:35:04

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [PATCH 11/11] drm/msm/dpu: do not use mixer that supports dspp when not required

On 19/04/2023 17:41, Arnaud Vrac wrote:
> This avoids using lm blocks that support DSPP when not needed, to
> keep those resources available.

This will break some of the platforms. Consider qcm2290 which has a
single LM with DSPP. So, _dpu_rm_check_lm_and_get_connected_blks should
be performed in two steps: first skip non-DSPP-enabled LMs when DSPP is
not required. Then, if the LM (pair) is not found, look for any suitable
LM(pair).

>
> Signed-off-by: Arnaud Vrac <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> index f4dda88a73f7d..4b393d46c743f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> @@ -362,7 +362,7 @@ static bool _dpu_rm_check_lm_and_get_connected_blks(struct dpu_rm *rm,
> *pp_idx = idx;
>
> if (!reqs->topology.num_dspp)
> - return true;
> + return !lm_cfg->dspp;
>
> idx = lm_cfg->dspp - DSPP_0;
> if (idx < 0 || idx >= ARRAY_SIZE(rm->dspp_blks)) {
>

--
With best wishes
Dmitry

2023-04-20 07:09:13

by Arnaud Vrac

[permalink] [raw]
Subject: Re: [PATCH 07/11] drm/msm/dpu: add sspp cursor blocks to msm8998 hw catalog

Le jeu. 20 avr. 2023 à 01:10, Dmitry Baryshkov
<[email protected]> a écrit :
>
> On 19/04/2023 17:41, Arnaud Vrac wrote:
> > Now that cursor sspp blocks can be used for cursor planes, enable them
> > on msm8998. The dma sspp blocks that were assigned to cursor planes can
> > now be used for overlay planes instead.
>
> While the change is correct, there is more about it. Composers, using
> universal planes, will see this plane too. They have no obligations to
> use it only for the cursor. At the minimum could you please extend the
> plane_atomic_check to check for the plane dimensions for the CURSOR pipes?

Hum, I had assumed the generic atomic checks would already do this,
but it's not the case. I'll add the check when the pipe is of type
SSPP_CURSOR in another patch coming before, thanks.

>
> For this change:
>
> Reviewed-by: Dmitry Baryshkov <[email protected]>
>
> >
> > Signed-off-by: Arnaud Vrac <[email protected]>
> > ---
> > .../drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h | 8 +++--
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 34 ++++++++++++++++++++++
> > 2 files changed, 40 insertions(+), 2 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 b07e8a9941f79..7de393b0f91d7 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
> > @@ -90,10 +90,14 @@ static const struct dpu_sspp_cfg msm8998_sspp[] = {
> > sdm845_dma_sblk_0, 1, SSPP_TYPE_DMA, DPU_CLK_CTRL_DMA0),
> > SSPP_BLK("sspp_9", SSPP_DMA1, 0x26000, 0x1ac, DMA_MSM8998_MASK,
> > sdm845_dma_sblk_1, 5, SSPP_TYPE_DMA, DPU_CLK_CTRL_DMA1),
> > - SSPP_BLK("sspp_10", SSPP_DMA2, 0x28000, 0x1ac, DMA_CURSOR_MSM8998_MASK,
> > + SSPP_BLK("sspp_10", SSPP_DMA2, 0x28000, 0x1ac, DMA_MSM8998_MASK,
> > sdm845_dma_sblk_2, 9, SSPP_TYPE_DMA, DPU_CLK_CTRL_DMA2),
> > - SSPP_BLK("sspp_11", SSPP_DMA3, 0x2a000, 0x1ac, DMA_CURSOR_MSM8998_MASK,
> > + SSPP_BLK("sspp_11", SSPP_DMA3, 0x2a000, 0x1ac, DMA_MSM8998_MASK,
> > sdm845_dma_sblk_3, 13, SSPP_TYPE_DMA, DPU_CLK_CTRL_DMA3),
> > + SSPP_BLK("sspp_12", SSPP_CURSOR0, 0x34000, 0x1ac, DMA_CURSOR_MSM8998_MASK,
> > + msm8998_cursor_sblk_0, 2, SSPP_TYPE_CURSOR, DPU_CLK_CTRL_CURSOR0),
> > + SSPP_BLK("sspp_13", SSPP_CURSOR1, 0x36000, 0x1ac, DMA_CURSOR_MSM8998_MASK,
> > + msm8998_cursor_sblk_1, 10, SSPP_TYPE_CURSOR, DPU_CLK_CTRL_CURSOR1),
> > };
> >
> > static const struct dpu_lm_cfg msm8998_lm[] = {
> > 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 8d5d782a43398..f34fa704936bc 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > @@ -242,6 +242,22 @@ static const uint32_t wb2_formats[] = {
> > DRM_FORMAT_XBGR4444,
> > };
> >
> > +static const uint32_t cursor_formats[] = {
> > + DRM_FORMAT_ARGB8888,
> > + DRM_FORMAT_ABGR8888,
> > + DRM_FORMAT_RGBA8888,
> > + DRM_FORMAT_BGRA8888,
> > + DRM_FORMAT_XRGB8888,
> > + DRM_FORMAT_ARGB1555,
> > + DRM_FORMAT_ABGR1555,
> > + DRM_FORMAT_RGBA5551,
> > + DRM_FORMAT_BGRA5551,
> > + DRM_FORMAT_ARGB4444,
> > + DRM_FORMAT_ABGR4444,
> > + DRM_FORMAT_RGBA4444,
> > + DRM_FORMAT_BGRA4444,
> > +};
> > +
> > /*************************************************************
> > * SSPP sub blocks config
> > *************************************************************/
> > @@ -300,6 +316,19 @@ static const uint32_t wb2_formats[] = {
> > .virt_num_formats = ARRAY_SIZE(plane_formats), \
> > }
> >
> > +#define _CURSOR_SBLK(num) \
> > + { \
> > + .maxdwnscale = SSPP_UNITY_SCALE, \
> > + .maxupscale = SSPP_UNITY_SCALE, \
> > + .smart_dma_priority = 0, \
> > + .src_blk = {.name = STRCAT("sspp_src_", num), \
> > + .id = DPU_SSPP_SRC, .base = 0x00, .len = 0x150,}, \
> > + .format_list = cursor_formats, \
> > + .num_formats = ARRAY_SIZE(cursor_formats), \
> > + .virt_format_list = cursor_formats, \
> > + .virt_num_formats = ARRAY_SIZE(cursor_formats), \
> > + }
> > +
> > static const struct dpu_sspp_sub_blks msm8998_vig_sblk_0 =
> > _VIG_SBLK("0", 0, DPU_SSPP_SCALER_QSEED3);
> > static const struct dpu_sspp_sub_blks msm8998_vig_sblk_1 =
> > @@ -309,6 +338,11 @@ static const struct dpu_sspp_sub_blks msm8998_vig_sblk_2 =
> > static const struct dpu_sspp_sub_blks msm8998_vig_sblk_3 =
> > _VIG_SBLK("3", 0, DPU_SSPP_SCALER_QSEED3);
> >
> > +static const struct dpu_sspp_sub_blks msm8998_cursor_sblk_0 =
> > + _CURSOR_SBLK("12");
> > +static const struct dpu_sspp_sub_blks msm8998_cursor_sblk_1 =
> > + _CURSOR_SBLK("13");
> > +
> > static const struct dpu_rotation_cfg dpu_rot_sc7280_cfg_v2 = {
> > .rot_maxheight = 1088,
> > .rot_num_formats = ARRAY_SIZE(rotation_v2_formats),
> >
>
> --
> With best wishes
> Dmitry
>

2023-04-20 07:10:05

by Arnaud Vrac

[permalink] [raw]
Subject: Re: [PATCH 11/11] drm/msm/dpu: do not use mixer that supports dspp when not required

Le jeu. 20 avr. 2023 à 01:18, Dmitry Baryshkov
<[email protected]> a écrit :
>
> On 19/04/2023 17:41, Arnaud Vrac wrote:
> > This avoids using lm blocks that support DSPP when not needed, to
> > keep those resources available.
>
> This will break some of the platforms. Consider qcm2290 which has a
> single LM with DSPP. So, _dpu_rm_check_lm_and_get_connected_blks should
> be performed in two steps: first skip non-DSPP-enabled LMs when DSPP is
> not required. Then, if the LM (pair) is not found, look for any suitable
> LM(pair).

Good point, I'll add the change.

>
> >
> > Signed-off-by: Arnaud Vrac <[email protected]>
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > index f4dda88a73f7d..4b393d46c743f 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > @@ -362,7 +362,7 @@ static bool _dpu_rm_check_lm_and_get_connected_blks(struct dpu_rm *rm,
> > *pp_idx = idx;
> >
> > if (!reqs->topology.num_dspp)
> > - return true;
> > + return !lm_cfg->dspp;
> >
> > idx = lm_cfg->dspp - DSPP_0;
> > if (idx < 0 || idx >= ARRAY_SIZE(rm->dspp_blks)) {
> >
>
> --
> With best wishes
> Dmitry
>

2023-04-20 07:28:20

by Arnaud Vrac

[permalink] [raw]
Subject: Re: [PATCH 04/11] drm/msm/dpu: allow using lm mixer base stage

Le jeu. 20 avr. 2023 à 00:43, Dmitry Baryshkov
<[email protected]> a écrit :
>
> On 19/04/2023 17:41, Arnaud Vrac wrote:
> > The dpu backend already handles applying alpha to the base stage, so we
> > can use it to render the bottom plane in all cases. This allows mixing
> > one additional plane with the hardware mixer.
> >
> > Signed-off-by: Arnaud Vrac <[email protected]>
>
> This might require additional changes. First, for the STAGE_BASE pipe
> in the source split mode (iow using two LMs) should programmed with
> respect to the right LM's x offset (rather than usual left top-left LM).
> See mdss_mdp_pipe_position_update().

Ok, I did test with 2 LMs and it seems to be working, I'll investigate.

>
> Also this might need some interaction with CTL_MIXER_BORDER_OUT being
> set or not. If I remember correctly, if there bottom plane is not
> fullscreen or if there are no planes at all, we should set
> CTL_MIXER_BORDER_OUT (which takes STAGE_BASE) and start assigning them
> from STAGE0. If not, we can use STAGE_BASE.

I also tested with both fullscreen and non-fullscreen primary plane,
and no plane. I'll check this.

>
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > index 14b5cfe306113..148921ed62f85 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > @@ -881,7 +881,7 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
> > r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> > r_pipe->sspp = NULL;
> >
> > - pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos;
> > + pstate->stage = DPU_STAGE_BASE + pstate->base.normalized_zpos;
> > if (pstate->stage >= pdpu->catalog->caps->max_mixer_blendstages) {
> > DPU_ERROR("> %d plane stages assigned\n",
> > pdpu->catalog->caps->max_mixer_blendstages - DPU_STAGE_0);
> >
>
> --
> With best wishes
> Dmitry
>

2023-04-20 08:55:24

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [PATCH 07/11] drm/msm/dpu: add sspp cursor blocks to msm8998 hw catalog

On Thu, 20 Apr 2023 at 10:06, Arnaud Vrac <[email protected]> wrote:
>
> Le jeu. 20 avr. 2023 à 01:10, Dmitry Baryshkov
> <[email protected]> a écrit :
> >
> > On 19/04/2023 17:41, Arnaud Vrac wrote:
> > > Now that cursor sspp blocks can be used for cursor planes, enable them
> > > on msm8998. The dma sspp blocks that were assigned to cursor planes can
> > > now be used for overlay planes instead.
> >
> > While the change is correct, there is more about it. Composers, using
> > universal planes, will see this plane too. They have no obligations to
> > use it only for the cursor. At the minimum could you please extend the
> > plane_atomic_check to check for the plane dimensions for the CURSOR pipes?
>
> Hum, I had assumed the generic atomic checks would already do this,

Atomic will have these checks for the legacy cursor API (using the
mode_config.cursor_width/cursor_height that you have added). But I
don't think there is a generic API for telling the core 'this plane is
slightly limited'.

Fortunately, once the virtual planes land and are taught about the
SSPP_CURSOR peculiarities, it should not matter, since the driver will
know that it should not select these pipes in the inappropriate cases.

> but it's not the case. I'll add the check when the pipe is of type
> SSPP_CURSOR in another patch coming before, thanks.
>
> >
> > For this change:
> >
> > Reviewed-by: Dmitry Baryshkov <[email protected]>
> >
> > >
> > > Signed-off-by: Arnaud Vrac <[email protected]>
> > > ---
> > > .../drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h | 8 +++--
> > > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 34 ++++++++++++++++++++++
> > > 2 files changed, 40 insertions(+), 2 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 b07e8a9941f79..7de393b0f91d7 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
> > > @@ -90,10 +90,14 @@ static const struct dpu_sspp_cfg msm8998_sspp[] = {
> > > sdm845_dma_sblk_0, 1, SSPP_TYPE_DMA, DPU_CLK_CTRL_DMA0),
> > > SSPP_BLK("sspp_9", SSPP_DMA1, 0x26000, 0x1ac, DMA_MSM8998_MASK,
> > > sdm845_dma_sblk_1, 5, SSPP_TYPE_DMA, DPU_CLK_CTRL_DMA1),
> > > - SSPP_BLK("sspp_10", SSPP_DMA2, 0x28000, 0x1ac, DMA_CURSOR_MSM8998_MASK,
> > > + SSPP_BLK("sspp_10", SSPP_DMA2, 0x28000, 0x1ac, DMA_MSM8998_MASK,
> > > sdm845_dma_sblk_2, 9, SSPP_TYPE_DMA, DPU_CLK_CTRL_DMA2),
> > > - SSPP_BLK("sspp_11", SSPP_DMA3, 0x2a000, 0x1ac, DMA_CURSOR_MSM8998_MASK,
> > > + SSPP_BLK("sspp_11", SSPP_DMA3, 0x2a000, 0x1ac, DMA_MSM8998_MASK,
> > > sdm845_dma_sblk_3, 13, SSPP_TYPE_DMA, DPU_CLK_CTRL_DMA3),
> > > + SSPP_BLK("sspp_12", SSPP_CURSOR0, 0x34000, 0x1ac, DMA_CURSOR_MSM8998_MASK,
> > > + msm8998_cursor_sblk_0, 2, SSPP_TYPE_CURSOR, DPU_CLK_CTRL_CURSOR0),
> > > + SSPP_BLK("sspp_13", SSPP_CURSOR1, 0x36000, 0x1ac, DMA_CURSOR_MSM8998_MASK,
> > > + msm8998_cursor_sblk_1, 10, SSPP_TYPE_CURSOR, DPU_CLK_CTRL_CURSOR1),
> > > };
> > >
> > > static const struct dpu_lm_cfg msm8998_lm[] = {
> > > 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 8d5d782a43398..f34fa704936bc 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> > > @@ -242,6 +242,22 @@ static const uint32_t wb2_formats[] = {
> > > DRM_FORMAT_XBGR4444,
> > > };
> > >
> > > +static const uint32_t cursor_formats[] = {
> > > + DRM_FORMAT_ARGB8888,
> > > + DRM_FORMAT_ABGR8888,
> > > + DRM_FORMAT_RGBA8888,
> > > + DRM_FORMAT_BGRA8888,
> > > + DRM_FORMAT_XRGB8888,
> > > + DRM_FORMAT_ARGB1555,
> > > + DRM_FORMAT_ABGR1555,
> > > + DRM_FORMAT_RGBA5551,
> > > + DRM_FORMAT_BGRA5551,
> > > + DRM_FORMAT_ARGB4444,
> > > + DRM_FORMAT_ABGR4444,
> > > + DRM_FORMAT_RGBA4444,
> > > + DRM_FORMAT_BGRA4444,
> > > +};
> > > +
> > > /*************************************************************
> > > * SSPP sub blocks config
> > > *************************************************************/
> > > @@ -300,6 +316,19 @@ static const uint32_t wb2_formats[] = {
> > > .virt_num_formats = ARRAY_SIZE(plane_formats), \
> > > }
> > >
> > > +#define _CURSOR_SBLK(num) \
> > > + { \
> > > + .maxdwnscale = SSPP_UNITY_SCALE, \
> > > + .maxupscale = SSPP_UNITY_SCALE, \
> > > + .smart_dma_priority = 0, \
> > > + .src_blk = {.name = STRCAT("sspp_src_", num), \
> > > + .id = DPU_SSPP_SRC, .base = 0x00, .len = 0x150,}, \
> > > + .format_list = cursor_formats, \
> > > + .num_formats = ARRAY_SIZE(cursor_formats), \
> > > + .virt_format_list = cursor_formats, \
> > > + .virt_num_formats = ARRAY_SIZE(cursor_formats), \
> > > + }
> > > +
> > > static const struct dpu_sspp_sub_blks msm8998_vig_sblk_0 =
> > > _VIG_SBLK("0", 0, DPU_SSPP_SCALER_QSEED3);
> > > static const struct dpu_sspp_sub_blks msm8998_vig_sblk_1 =
> > > @@ -309,6 +338,11 @@ static const struct dpu_sspp_sub_blks msm8998_vig_sblk_2 =
> > > static const struct dpu_sspp_sub_blks msm8998_vig_sblk_3 =
> > > _VIG_SBLK("3", 0, DPU_SSPP_SCALER_QSEED3);
> > >
> > > +static const struct dpu_sspp_sub_blks msm8998_cursor_sblk_0 =
> > > + _CURSOR_SBLK("12");
> > > +static const struct dpu_sspp_sub_blks msm8998_cursor_sblk_1 =
> > > + _CURSOR_SBLK("13");
> > > +
> > > static const struct dpu_rotation_cfg dpu_rot_sc7280_cfg_v2 = {
> > > .rot_maxheight = 1088,
> > > .rot_num_formats = ARRAY_SIZE(rotation_v2_formats),
> > >
> >
> > --
> > With best wishes
> > Dmitry
> >



--
With best wishes
Dmitry

2023-04-20 10:00:49

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [PATCH 04/11] drm/msm/dpu: allow using lm mixer base stage

On 20/04/2023 10:26, Arnaud Vrac wrote:
> Le jeu. 20 avr. 2023 à 00:43, Dmitry Baryshkov
> <[email protected]> a écrit :
>>
>> On 19/04/2023 17:41, Arnaud Vrac wrote:
>>> The dpu backend already handles applying alpha to the base stage, so we
>>> can use it to render the bottom plane in all cases. This allows mixing
>>> one additional plane with the hardware mixer.
>>>
>>> Signed-off-by: Arnaud Vrac <[email protected]>
>>
>> This might require additional changes. First, for the STAGE_BASE pipe
>> in the source split mode (iow using two LMs) should programmed with
>> respect to the right LM's x offset (rather than usual left top-left LM).
>> See mdss_mdp_pipe_position_update().
>
> Ok, I did test with 2 LMs and it seems to be working, I'll investigate.

The only reference I have here is the fbdev driver, see [1]. The newer
SDE driver doesn't handle STAGE_BASE vs STAGE0 (and DPU inherited that
design). Maybe this got fixed in hw at some point.

[1]
https://git.codelinaro.org/clo/la/kernel/msm-4.19/-/blob/LE.UM.4.4.1.r2-17500-QRB5165.0/drivers/video/fbdev/msm/mdss_mdp_pipe.c#L1789

I think, it only concerns the src_split + multirect cases, where the
rectangle base point is on the right LM.

>
>>
>> Also this might need some interaction with CTL_MIXER_BORDER_OUT being
>> set or not. If I remember correctly, if there bottom plane is not
>> fullscreen or if there are no planes at all, we should set
>> CTL_MIXER_BORDER_OUT (which takes STAGE_BASE) and start assigning them
>> from STAGE0. If not, we can use STAGE_BASE.
>
> I also tested with both fullscreen and non-fullscreen primary plane,
> and no plane. I'll check this.

Yes, the DPU driver always enables the MIXER_BORDER_OUT.

>
>>
>>> ---
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>> index 14b5cfe306113..148921ed62f85 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
>>> @@ -881,7 +881,7 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
>>> r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
>>> r_pipe->sspp = NULL;
>>>
>>> - pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos;
>>> + pstate->stage = DPU_STAGE_BASE + pstate->base.normalized_zpos;
>>> if (pstate->stage >= pdpu->catalog->caps->max_mixer_blendstages) {
>>> DPU_ERROR("> %d plane stages assigned\n",
>>> pdpu->catalog->caps->max_mixer_blendstages - DPU_STAGE_0);
>>>
>>
>> --
>> With best wishes
>> Dmitry
>>

--
With best wishes
Dmitry

2023-04-20 17:56:45

by Jeykumar Sankaran

[permalink] [raw]
Subject: Re: [Freedreno] [PATCH 02/11] drm/msm/dpu: use the actual lm maximum width instead of a hardcoded value



On 4/19/2023 3:23 PM, Dmitry Baryshkov wrote:
> On 19/04/2023 17:41, Arnaud Vrac wrote:
>> This avoids using two LMs instead of one when the display width is lower
>> than the maximum supported value. For example on MSM8996/MSM8998, the
>> actual maxwidth is 2560, so we would use two LMs for 1280x720 or
>> 1920x1080 resolutions, while one is enough.
>>
>> Signed-off-by: Arnaud Vrac <[email protected]>
>
> While this looks correct (and following what we have in 4.4), later
> vendor kernels specify the topology explicitly. Probably we should check
> this with the hw guys, because it might be the following case: even
> though a single LM can supply the mode, it will spend more power
> compared to two LMs.
>
>
Yes. 2 LM split will allow the HW to run in lower mdp core clock. Can
you maintain the split_threshold in the hw catalog until per mode
topology is available?

Jeykumar S
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 1dc5dbe585723..dd2914726c4f6 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -53,8 +53,6 @@
>>   #define IDLE_SHORT_TIMEOUT    1
>> -#define MAX_HDISPLAY_SPLIT 1080
>> -
>>   /* timeout in frames waiting for frame done */
>>   #define DPU_ENCODER_FRAME_DONE_TIMEOUT_FRAMES 5
>> @@ -568,10 +566,12 @@ static struct msm_display_topology
>> dpu_encoder_get_topology(
>>        */
>>       if (intf_count == 2)
>>           topology.num_lm = 2;
>> -    else if (!dpu_kms->catalog->caps->has_3d_merge)
>> -        topology.num_lm = 1;
>> +    else if (dpu_kms->catalog->caps->has_3d_merge &&
>> +         dpu_kms->catalog->mixer_count > 0 &&
>> +         mode->hdisplay > dpu_kms->catalog->mixer[0].sblk->maxwidth)
>> +        topology.num_lm = 2;
>>       else
>> -        topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 : 1;
>> +        topology.num_lm = 1;
>>       if (crtc_state->ctm)
>>           topology.num_dspp = topology.num_lm;
>>
>

2023-04-20 18:11:58

by Jeykumar Sankaran

[permalink] [raw]
Subject: Re: [Freedreno] [PATCH 03/11] drm/msm/dpu: use hsync/vsync polarity set by the encoder



On 4/19/2023 7:41 AM, Arnaud Vrac wrote:
> Do not override the hsync/vsync polarity passed by the encoder when
> setting up intf timings. The same logic was used in both the encoder and
> intf code to set the DP and DSI polarities, so those interfaces are not
> impacted. However for HDMI, the polarities were overriden to static
> values based on the vertical resolution, instead of using the actual
> mode polarities.
>
Any idea why vres based static polarity override was in place? Hope you
had tested HDMI resolutions with yres > and < than 720.

Jeykumar S.

> Signed-off-by: Arnaud Vrac <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 16 +++-------------
> 1 file changed, 3 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> index 84ee2efa9c664..9f05417eb1213 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -104,7 +104,7 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
> u32 active_h_start, active_h_end;
> u32 active_v_start, active_v_end;
> u32 active_hctl, display_hctl, hsync_ctl;
> - u32 polarity_ctl, den_polarity, hsync_polarity, vsync_polarity;
> + u32 polarity_ctl, den_polarity;
> u32 panel_format;
> u32 intf_cfg, intf_cfg2 = 0;
> u32 display_data_hctl = 0, active_data_hctl = 0;
> @@ -191,19 +191,9 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
> }
>
> den_polarity = 0;
> - if (ctx->cap->type == INTF_HDMI) {
> - hsync_polarity = p->yres >= 720 ? 0 : 1;
> - vsync_polarity = p->yres >= 720 ? 0 : 1;
> - } else if (ctx->cap->type == INTF_DP) {
> - hsync_polarity = p->hsync_polarity;
> - vsync_polarity = p->vsync_polarity;
> - } else {
> - hsync_polarity = 0;
> - vsync_polarity = 0;
> - }
> polarity_ctl = (den_polarity << 2) | /* DEN Polarity */
> - (vsync_polarity << 1) | /* VSYNC Polarity */
> - (hsync_polarity << 0); /* HSYNC Polarity */
> + (p->vsync_polarity << 1) | /* VSYNC Polarity */
> + (p->hsync_polarity << 0); /* HSYNC Polarity */
>
> if (!DPU_FORMAT_IS_YUV(fmt))
> panel_format = (fmt->bits[C0_G_Y] |
>

2023-04-20 18:52:04

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [Freedreno] [PATCH 03/11] drm/msm/dpu: use hsync/vsync polarity set by the encoder

On Thu, 20 Apr 2023 at 21:01, Jeykumar Sankaran
<[email protected]> wrote:
>
>
>
> On 4/19/2023 7:41 AM, Arnaud Vrac wrote:
> > Do not override the hsync/vsync polarity passed by the encoder when
> > setting up intf timings. The same logic was used in both the encoder and
> > intf code to set the DP and DSI polarities, so those interfaces are not
> > impacted. However for HDMI, the polarities were overriden to static
> > values based on the vertical resolution, instead of using the actual
> > mode polarities.
> >
> Any idea why vres based static polarity override was in place? Hope you
> had tested HDMI resolutions with yres > and < than 720.

I think it migrated from the old fbdev, see [1].
Tthis easily causes issues with the DVI monitors, which have different
rules for hsync/vsync. I have observed this on other platforms.
In practice it was observed that EDID provides correct sync polarity,
so it is better to follow the EDID data. If for some reason EDID is
incorrect, DRM provides a way to override it.

[1] https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/commit/29c3d616bf3f89f13bbab46c58151ae7a3a79b98

>
> Jeykumar S.
>
> > Signed-off-by: Arnaud Vrac <[email protected]>
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 16 +++-------------
> > 1 file changed, 3 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> > index 84ee2efa9c664..9f05417eb1213 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> > @@ -104,7 +104,7 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
> > u32 active_h_start, active_h_end;
> > u32 active_v_start, active_v_end;
> > u32 active_hctl, display_hctl, hsync_ctl;
> > - u32 polarity_ctl, den_polarity, hsync_polarity, vsync_polarity;
> > + u32 polarity_ctl, den_polarity;
> > u32 panel_format;
> > u32 intf_cfg, intf_cfg2 = 0;
> > u32 display_data_hctl = 0, active_data_hctl = 0;
> > @@ -191,19 +191,9 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
> > }
> >
> > den_polarity = 0;
> > - if (ctx->cap->type == INTF_HDMI) {
> > - hsync_polarity = p->yres >= 720 ? 0 : 1;
> > - vsync_polarity = p->yres >= 720 ? 0 : 1;
> > - } else if (ctx->cap->type == INTF_DP) {
> > - hsync_polarity = p->hsync_polarity;
> > - vsync_polarity = p->vsync_polarity;
> > - } else {
> > - hsync_polarity = 0;
> > - vsync_polarity = 0;
> > - }
> > polarity_ctl = (den_polarity << 2) | /* DEN Polarity */
> > - (vsync_polarity << 1) | /* VSYNC Polarity */
> > - (hsync_polarity << 0); /* HSYNC Polarity */
> > + (p->vsync_polarity << 1) | /* VSYNC Polarity */
> > + (p->hsync_polarity << 0); /* HSYNC Polarity */
> >
> > if (!DPU_FORMAT_IS_YUV(fmt))
> > panel_format = (fmt->bits[C0_G_Y] |
> >



--
With best wishes
Dmitry

2023-04-25 01:07:02

by Abhinav Kumar

[permalink] [raw]
Subject: Re: [PATCH 10/11] drm/msm/dpu: tweak lm pairings in msm8998 hw catalog



On 4/19/2023 7:41 AM, Arnaud Vrac wrote:
> Change lm blocks pairs so that lm blocks with the same features are
> paired together:
>
> LM_0 and LM_1 with PP and DSPP
> LM_2 and LM_5 with PP
> LM_3 and LM_4
>
> This matches the sdm845 configuration and allows using pp or dspp when 2
> lm blocks are needed in the topology. In the previous config the
> reservation code could never find an lm pair without a matching feature
> set.
>
> Signed-off-by: Arnaud Vrac <[email protected]>

Reviewed-by: Abhinav Kumar <[email protected]>

2023-04-25 21:59:53

by Abhinav Kumar

[permalink] [raw]
Subject: Re: [PATCH 01/11] drm/msm/dpu: tweak msm8998 hw catalog values



On 4/19/2023 7:41 AM, Arnaud Vrac wrote:
> Match the values found in the downstream msm-4.4 kernel sde driver.
>
> Signed-off-by: Arnaud Vrac <[email protected]>
> ---

Reviewed-by: Abhinav Kumar <[email protected]>

2023-04-28 20:27:51

by Abhinav Kumar

[permalink] [raw]
Subject: Re: (subset) [PATCH 00/11] drm/msm/dpu: tweaks for better hardware resources allocation


On Wed, 19 Apr 2023 16:41:07 +0200, Arnaud Vrac wrote:
> This series include misc fixes related to hardware resource allocations
> in the msm dpu driver, some specifically for msm8998 (including hw
> catalog fixes and cursor sspp support for cursor planes, instead of
> using Smart DMA pipes).
>
> This series has been tested on msm8998 with additional patches to enable
> hdmi support.
>
> [...]

Applied, thanks!

[01/11] drm/msm/dpu: tweak msm8998 hw catalog values
https://gitlab.freedesktop.org/abhinavk/msm/-/commit/3f23a52fc2b8
[10/11] drm/msm/dpu: tweak lm pairings in msm8998 hw catalog
https://gitlab.freedesktop.org/abhinavk/msm/-/commit/686eb89b1036

Best regards,
--
Abhinav Kumar <[email protected]>

2023-05-20 21:10:18

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [Freedreno] [PATCH 02/11] drm/msm/dpu: use the actual lm maximum width instead of a hardcoded value

On 20/04/2023 20:47, Jeykumar Sankaran wrote:
>
>
> On 4/19/2023 3:23 PM, Dmitry Baryshkov wrote:
>> On 19/04/2023 17:41, Arnaud Vrac wrote:
>>> This avoids using two LMs instead of one when the display width is lower
>>> than the maximum supported value. For example on MSM8996/MSM8998, the
>>> actual maxwidth is 2560, so we would use two LMs for 1280x720 or
>>> 1920x1080 resolutions, while one is enough.
>>>
>>> Signed-off-by: Arnaud Vrac <[email protected]>
>>
>> While this looks correct (and following what we have in 4.4), later
>> vendor kernels specify the topology explicitly. Probably we should
>> check this with the hw guys, because it might be the following case:
>> even though a single LM can supply the mode, it will spend more power
>> compared to two LMs.
>>
>>
> Yes. 2 LM split will allow the HW to run in lower mdp core clock. Can
> you maintain the split_threshold in the hw catalog until per mode
> topology is available?

I don't think it warrants the trouble, unless we have a real usecase
when the device is short of LMs.

Arnaud, I'll mark this patch as Rejected for now, unless it fixes an LM
shortage for your platform.

>
> Jeykumar S
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 10 +++++-----
>>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> index 1dc5dbe585723..dd2914726c4f6 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> @@ -53,8 +53,6 @@
>>>   #define IDLE_SHORT_TIMEOUT    1
>>> -#define MAX_HDISPLAY_SPLIT 1080
>>> -
>>>   /* timeout in frames waiting for frame done */
>>>   #define DPU_ENCODER_FRAME_DONE_TIMEOUT_FRAMES 5
>>> @@ -568,10 +566,12 @@ static struct msm_display_topology
>>> dpu_encoder_get_topology(
>>>        */
>>>       if (intf_count == 2)
>>>           topology.num_lm = 2;
>>> -    else if (!dpu_kms->catalog->caps->has_3d_merge)
>>> -        topology.num_lm = 1;
>>> +    else if (dpu_kms->catalog->caps->has_3d_merge &&
>>> +         dpu_kms->catalog->mixer_count > 0 &&
>>> +         mode->hdisplay > dpu_kms->catalog->mixer[0].sblk->maxwidth)
>>> +        topology.num_lm = 2;
>>>       else
>>> -        topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2
>>> : 1;
>>> +        topology.num_lm = 1;
>>>       if (crtc_state->ctm)
>>>           topology.num_dspp = topology.num_lm;
>>>
>>

--
With best wishes
Dmitry


2023-05-22 09:54:12

by Arnaud Vrac

[permalink] [raw]
Subject: Re: [Freedreno] [PATCH 02/11] drm/msm/dpu: use the actual lm maximum width instead of a hardcoded value

Le sam. 20 mai 2023 à 22:49, Dmitry Baryshkov
<[email protected]> a écrit :
>
> On 20/04/2023 20:47, Jeykumar Sankaran wrote:
> >
> >
> > On 4/19/2023 3:23 PM, Dmitry Baryshkov wrote:
> >> On 19/04/2023 17:41, Arnaud Vrac wrote:
> >>> This avoids using two LMs instead of one when the display width is lower
> >>> than the maximum supported value. For example on MSM8996/MSM8998, the
> >>> actual maxwidth is 2560, so we would use two LMs for 1280x720 or
> >>> 1920x1080 resolutions, while one is enough.
> >>>
> >>> Signed-off-by: Arnaud Vrac <[email protected]>
> >>
> >> While this looks correct (and following what we have in 4.4), later
> >> vendor kernels specify the topology explicitly. Probably we should
> >> check this with the hw guys, because it might be the following case:
> >> even though a single LM can supply the mode, it will spend more power
> >> compared to two LMs.
> >>
> >>
> > Yes. 2 LM split will allow the HW to run in lower mdp core clock. Can
> > you maintain the split_threshold in the hw catalog until per mode
> > topology is available?
>
> I don't think it warrants the trouble, unless we have a real usecase
> when the device is short of LMs.
>
> Arnaud, I'll mark this patch as Rejected for now, unless it fixes an LM
> shortage for your platform.

It's fine, if I remember correctly I wrote this patch because display
wouldn't work before I fixed the LM pairings on msm8998, but now it's
not a requirement anymore.

>
> >
> > Jeykumar S
> >>> ---
> >>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 10 +++++-----
> >>> 1 file changed, 5 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >>> index 1dc5dbe585723..dd2914726c4f6 100644
> >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >>> @@ -53,8 +53,6 @@
> >>> #define IDLE_SHORT_TIMEOUT 1
> >>> -#define MAX_HDISPLAY_SPLIT 1080
> >>> -
> >>> /* timeout in frames waiting for frame done */
> >>> #define DPU_ENCODER_FRAME_DONE_TIMEOUT_FRAMES 5
> >>> @@ -568,10 +566,12 @@ static struct msm_display_topology
> >>> dpu_encoder_get_topology(
> >>> */
> >>> if (intf_count == 2)
> >>> topology.num_lm = 2;
> >>> - else if (!dpu_kms->catalog->caps->has_3d_merge)
> >>> - topology.num_lm = 1;
> >>> + else if (dpu_kms->catalog->caps->has_3d_merge &&
> >>> + dpu_kms->catalog->mixer_count > 0 &&
> >>> + mode->hdisplay > dpu_kms->catalog->mixer[0].sblk->maxwidth)
> >>> + topology.num_lm = 2;
> >>> else
> >>> - topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2
> >>> : 1;
> >>> + topology.num_lm = 1;
> >>> if (crtc_state->ctm)
> >>> topology.num_dspp = topology.num_lm;
> >>>
> >>
>
> --
> With best wishes
> Dmitry
>