[PATCH 1/3] drm/msm/dpu: fix DSC for DSI video mode

[PATCH 1/3] drm/msm/dpu: fix DSC for DSI video mode

[Jun Nie]

Fix DSC timing and control configurations in DPU for DSI video mode.
Only compression ratio 3:1 is handled and tested.

This patch is modified from patchs of Jonathan Marek.

Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   |  2 +-
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  2 +-
 .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 12 +++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c   | 10 +++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h   |  1 +
 drivers/gpu/drm/msm/dsi/dsi.xml.h             |  1 +
 drivers/gpu/drm/msm/dsi/dsi_host.c            | 48 +++++++++++--------
 include/drm/display/drm_dsc.h                 |  4 ++
 8 files changed, 56 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 6a4b489d44e5..c1b9da06dde2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2440,7 +2440,7 @@ enum dpu_intf_mode dpu_encoder_get_intf_mode(struct drm_encoder *encoder)
 	return INTF_MODE_NONE;
 }
 
-unsigned int dpu_encoder_helper_get_dsc(struct dpu_encoder_phys *phys_enc)
+unsigned int dpu_encoder_helper_get_dsc(const struct dpu_encoder_phys *phys_enc)
 {
 	struct drm_encoder *encoder = phys_enc->parent;
 	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(encoder);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
index 993f26343331..5000fa22ad40 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -339,7 +339,7 @@ static inline enum dpu_3d_blend_mode dpu_encoder_helper_get_3d_blend_mode(
  *   used for this encoder.
  * @phys_enc: Pointer to physical encoder structure
  */
-unsigned int dpu_encoder_helper_get_dsc(struct dpu_encoder_phys *phys_enc);
+unsigned int dpu_encoder_helper_get_dsc(const struct dpu_encoder_phys *phys_enc);
 
 /**
  * dpu_encoder_helper_split_config - split display configuration helper function
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index d0f56c5c4cce..c0ff39450e66 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -102,6 +102,8 @@ static void drm_mode_to_intf_timing_params(
 	}
 
 	timing->wide_bus_en = dpu_encoder_is_widebus_enabled(phys_enc->parent);
+	if (dpu_encoder_helper_get_dsc(phys_enc))
+		timing->dsc_en = true;
 
 	/*
 	 * for DP, divide the horizonal parameters by 2 when
@@ -114,6 +116,16 @@ static void drm_mode_to_intf_timing_params(
 		timing->h_front_porch = timing->h_front_porch >> 1;
 		timing->hsync_pulse_width = timing->hsync_pulse_width >> 1;
 	}
+
+	/*
+	 * for DSI, if compression is enabled, then divide the horizonal active
+	 * timing parameters by compression ratio.
+	 */
+	if (phys_enc->hw_intf->cap->type != INTF_DP && timing->dsc_en) {
+		/* TODO: handle non 3:1 compression ratio, such as 30bpp case */
+		timing->width = timing->width / 3;
+		timing->xres = timing->width;
+	}
 }
 
 static u32 get_horizontal_total(const struct dpu_hw_intf_timing_params *timing)
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 6bba531d6dc4..e2f6fa542883 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -168,10 +168,18 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
 	 * video timing. It is recommended to enable it for all cases, except
 	 * if compression is enabled in 1 pixel per clock mode
 	 */
+	if (!p->dsc_en || p->wide_bus_en)
+		intf_cfg2 |= INTF_CFG2_DATA_HCTL_EN;
+
 	if (p->wide_bus_en)
-		intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN | INTF_CFG2_DATA_HCTL_EN;
+		intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
+
+	if (p->dsc_en)
+		intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
 
 	data_width = p->width;
+	if (p->wide_bus_en && !dp_intf)
+		data_width = p->width >> 1;
 
 	hsync_data_start_x = hsync_start_x;
 	hsync_data_end_x =  hsync_start_x + data_width - 1;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
index 0bd57a32144a..b452e3557d10 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
@@ -33,6 +33,7 @@ struct dpu_hw_intf_timing_params {
 	u32 hsync_skew;
 
 	bool wide_bus_en;
+	bool dsc_en;
 };
 
 struct dpu_hw_intf_prog_fetch {
diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h b/drivers/gpu/drm/msm/dsi/dsi.xml.h
index 2a7d980e12c3..f0b3cdc020a1 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
@@ -231,6 +231,7 @@ static inline uint32_t DSI_VID_CFG0_TRAFFIC_MODE(enum dsi_traffic_mode val)
 #define DSI_VID_CFG0_HSA_POWER_STOP				0x00010000
 #define DSI_VID_CFG0_HBP_POWER_STOP				0x00100000
 #define DSI_VID_CFG0_HFP_POWER_STOP				0x01000000
+#define DSI_VID_CFG0_DATABUS_WIDEN				0x02000000
 #define DSI_VID_CFG0_PULSE_MODE_HSA_HE				0x10000000
 
 #define REG_DSI_VID_CFG1					0x0000001c
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index deeecdfd6c4e..070a9aaa38d5 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -745,6 +745,8 @@ static void dsi_ctrl_enable(struct msm_dsi_host *msm_host,
 		data |= DSI_VID_CFG0_TRAFFIC_MODE(dsi_get_traffic_mode(flags));
 		data |= DSI_VID_CFG0_DST_FORMAT(dsi_get_vid_fmt(mipi_fmt));
 		data |= DSI_VID_CFG0_VIRT_CHANNEL(msm_host->channel);
+		if (msm_dsi_host_is_wide_bus_enabled(&msm_host->base))
+			data |= DSI_VID_CFG0_DATABUS_WIDEN;
 		dsi_write(msm_host, REG_DSI_VID_CFG0, data);
 
 		/* Do not swap RGB colors */
@@ -847,6 +849,8 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
 	u32 slice_per_intf, total_bytes_per_intf;
 	u32 pkt_per_line;
 	u32 eol_byte_num;
+	u32 bytes_per_pkt;
+	u32 slice_per_pkt;
 
 	/* first calculate dsc parameters and then program
 	 * compress mode registers
@@ -857,13 +861,12 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
 
 	eol_byte_num = total_bytes_per_intf % 3;
 
-	/*
-	 * Typically, pkt_per_line = slice_per_intf * slice_per_pkt.
-	 *
-	 * Since the current driver only supports slice_per_pkt = 1,
-	 * pkt_per_line will be equal to slice per intf for now.
-	 */
-	pkt_per_line = slice_per_intf;
+	/* If slice_per_pkt is greater than slice_per_intf then to 1 */
+	slice_per_pkt  = dsc->slice_per_pkt > slice_per_intf ?
+			 1 : dsc->slice_per_pkt;
+
+	bytes_per_pkt = dsc->slice_chunk_size * slice_per_pkt;
+	pkt_per_line = slice_per_intf / slice_per_pkt;
 
 	if (is_cmd_mode) /* packet data type */
 		reg = DSI_COMMAND_COMPRESSION_MODE_CTRL_STREAM0_DATATYPE(MIPI_DSI_DCS_LONG_WRITE);
@@ -873,6 +876,8 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
 	/* DSI_VIDEO_COMPRESSION_MODE & DSI_COMMAND_COMPRESSION_MODE
 	 * registers have similar offsets, so for below common code use
 	 * DSI_VIDEO_COMPRESSION_MODE_XXXX for setting bits
+	 *
+	 * pkt_per_line is log2 encoded, >>1 works for supported values (1,2,4)
 	 */
 	reg |= DSI_VIDEO_COMPRESSION_MODE_CTRL_PKT_PER_LINE(pkt_per_line >> 1);
 	reg |= DSI_VIDEO_COMPRESSION_MODE_CTRL_EOL_BYTE_NUM(eol_byte_num);
@@ -891,6 +896,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
 		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL, reg_ctrl);
 		dsi_write(msm_host, REG_DSI_COMMAND_COMPRESSION_MODE_CTRL2, reg_ctrl2);
 	} else {
+		reg |= DSI_VIDEO_COMPRESSION_MODE_CTRL_WC(bytes_per_pkt);
 		dsi_write(msm_host, REG_DSI_VIDEO_COMPRESSION_MODE_CTRL, reg);
 	}
 }
@@ -898,6 +904,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
 static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 {
 	struct drm_display_mode *mode = msm_host->mode;
+	struct drm_dsc_config *dsc = msm_host->dsc;
 	u32 hs_start = 0, vs_start = 0; /* take sync start as 0 */
 	u32 h_total = mode->htotal;
 	u32 v_total = mode->vtotal;
@@ -929,8 +936,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 		hdisplay /= 2;
 	}
 
-	if (msm_host->dsc) {
-		struct drm_dsc_config *dsc = msm_host->dsc;
+	if (dsc) {
 		u32 bytes_per_pclk;
 
 		/* update dsc params with timing params */
@@ -967,7 +973,7 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 	}
 
 	if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) {
-		if (msm_host->dsc)
+		if (dsc)
 			dsi_update_dsc_timing(msm_host, false, mode->hdisplay);
 
 		dsi_write(msm_host, REG_DSI_ACTIVE_H,
@@ -988,21 +994,17 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 			DSI_ACTIVE_VSYNC_VPOS_START(vs_start) |
 			DSI_ACTIVE_VSYNC_VPOS_END(vs_end));
 	} else {		/* command mode */
-		if (msm_host->dsc)
+		if (dsc) {
 			dsi_update_dsc_timing(msm_host, true, mode->hdisplay);
 
-		/* image data and 1 byte write_memory_start cmd */
-		if (!msm_host->dsc)
-			wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
-		else
 			/*
 			 * When DSC is enabled, WC = slice_chunk_size * slice_per_pkt + 1.
-			 * Currently, the driver only supports default value of slice_per_pkt = 1
-			 *
-			 * TODO: Expand mipi_dsi_device struct to hold slice_per_pkt info
-			 *       and adjust DSC math to account for slice_per_pkt.
 			 */
-			wc = msm_host->dsc->slice_chunk_size + 1;
+			wc = msm_host->dsc->slice_chunk_size * msm_host->dsc->slice_per_pkt + 1;
+		} else {
+			/* image data and 1 byte write_memory_start cmd */
+			wc = hdisplay * dsi_get_bpp(msm_host->format) / 8 + 1;
+		}
 
 		dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_CTRL,
 			DSI_CMD_MDP_STREAM0_CTRL_WORD_COUNT(wc) |
@@ -1629,8 +1631,12 @@ static int dsi_host_attach(struct mipi_dsi_host *host,
 	msm_host->lanes = dsi->lanes;
 	msm_host->format = dsi->format;
 	msm_host->mode_flags = dsi->mode_flags;
-	if (dsi->dsc)
+	if (dsi->dsc) {
 		msm_host->dsc = dsi->dsc;
+		/* for backwards compatibility, assume 1 if not set */
+		if (!msm_host->dsc->slice_per_pkt)
+			msm_host->dsc->slice_per_pkt = 1;
+	}
 
 	/* Some gpios defined in panel DT need to be controlled by host */
 	ret = dsi_host_init_panel_gpios(msm_host, &dsi->dev);
diff --git a/include/drm/display/drm_dsc.h b/include/drm/display/drm_dsc.h
index bc90273d06a6..01642bc9f016 100644
--- a/include/drm/display/drm_dsc.h
+++ b/include/drm/display/drm_dsc.h
@@ -92,6 +92,10 @@ struct drm_dsc_config {
 	 * @slice_count: Number fo slices per line used by the DSC encoder
 	 */
 	u8 slice_count;
+	/**
+	 * @slice_per_pkt: Number fo slices per packet in the encoded bit stream
+	 */
+	u8 slice_per_pkt;
 	/**
 	 *  @slice_width: Width of each slice in pixels
 	 */
-- 
2.34.1

[PATCH 2/3] dt-bindings: display: panel: visionox, vtdr6130: Add mode property

[Jun Nie]

Add DSI mode property and compression mode property

Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
 .../bindings/display/panel/visionox,vtdr6130.yaml         | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/panel/visionox,vtdr6130.yaml b/Documentation/devicetree/bindings/display/panel/visionox,vtdr6130.yaml
index d5a8295106c1..36ae94fa96ec 100644
--- a/Documentation/devicetree/bindings/display/panel/visionox,vtdr6130.yaml
+++ b/Documentation/devicetree/bindings/display/panel/visionox,vtdr6130.yaml
@@ -26,6 +26,14 @@ properties:
   port: true
   reset-gpios: true
 
+  enforce-cmd-mode:
+    type: boolean
+    description: Set DSI as command mode. Video mode by default.
+
+  enable-dsc:
+    type: boolean
+    description: Enable display stream compression
+
 additionalProperties: false
 
 required:
-- 
2.34.1

[PATCH 2/3] dt-bindings: display: panel: visionox,vtdr6130: Add mode property

[Jun Nie]

Add DSI mode property and compression mode property

Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
 .../bindings/display/panel/visionox,vtdr6130.yaml         | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/panel/visionox,vtdr6130.yaml b/Documentation/devicetree/bindings/display/panel/visionox,vtdr6130.yaml
index d5a8295106c1..36ae94fa96ec 100644
--- a/Documentation/devicetree/bindings/display/panel/visionox,vtdr6130.yaml
+++ b/Documentation/devicetree/bindings/display/panel/visionox,vtdr6130.yaml
@@ -26,6 +26,14 @@ properties:
   port: true
   reset-gpios: true
 
+  enforce-cmd-mode:
+    type: boolean
+    description: Set DSI as command mode. Video mode by default.
+
+  enable-dsc:
+    type: boolean
+    description: Enable display stream compression
+
 additionalProperties: false
 
 required:
-- 
2.34.1

[PATCH 3/3] drm/panel: Enable DSC for Visionox VTDR6130 panel

[Jun Nie]

Enable display compression (DSC v1.2) for 1080x2400 Visionox
VTDR6130 AMOLED DSI panel. DTS property is needed to enable DSC.
Default configuration is video mode + non-DSC for any back
compatibility.

Below modes works on SM8650. While cmd mode does not work yet.
- video mode + DSC
- video mode
- cmd mode + DSC

Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
 .../gpu/drm/panel/panel-visionox-vtdr6130.c   | 58 ++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
index 540099253e1b..d6c44816cdd0 100644
--- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
+++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
@@ -9,6 +9,7 @@
 #include <linux/of.h>
 
 #include <drm/display/drm_dsc.h>
+#include <drm/display/drm_dsc_helper.h>
 #include <drm/drm_mipi_dsi.h>
 #include <drm/drm_modes.h>
 #include <drm/drm_panel.h>
@@ -17,9 +18,12 @@
 
 struct visionox_vtdr6130 {
 	struct drm_panel panel;
+	struct drm_dsc_config dsc;
 	struct mipi_dsi_device *dsi;
 	struct gpio_desc *reset_gpio;
 	struct regulator_bulk_data supplies[3];
+	bool cmd_mode;
+	bool dsc_enable;
 };
 
 static inline struct visionox_vtdr6130 *to_visionox_vtdr6130(struct drm_panel *panel)
@@ -49,12 +53,20 @@ static int visionox_vtdr6130_on(struct visionox_vtdr6130 *ctx)
 	if (ret)
 		return ret;
 
+	if (ctx->dsc_enable)
+		mipi_dsi_dcs_write_seq(dsi, 0x03, 0x01);
+
 	mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, 0x20);
 	mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS, 0x00, 0x00);
 	mipi_dsi_dcs_write_seq(dsi, 0x59, 0x09);
 	mipi_dsi_dcs_write_seq(dsi, 0x6c, 0x01);
 	mipi_dsi_dcs_write_seq(dsi, 0x6d, 0x00);
-	mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01);
+
+	if (ctx->cmd_mode)
+		mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x02);
+	else
+		mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01);
+
 	mipi_dsi_dcs_write_seq(dsi, 0x70,
 			       0x12, 0x00, 0x00, 0xab, 0x30, 0x80, 0x09, 0x60, 0x04,
 			       0x38, 0x00, 0x28, 0x02, 0x1c, 0x02, 0x1c, 0x02, 0x00,
@@ -205,6 +217,26 @@ static const struct drm_display_mode visionox_vtdr6130_mode = {
 	.height_mm = 157,
 };
 
+static int visionox_vtdr6130_enable(struct drm_panel *panel)
+{
+	struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel);
+	struct mipi_dsi_device *dsi = ctx->dsi;
+	struct drm_dsc_picture_parameter_set pps;
+	int ret;
+
+	if (!dsi->dsc)
+		return 0;
+
+	drm_dsc_pps_payload_pack(&pps, dsi->dsc);
+	ret = mipi_dsi_picture_parameter_set(dsi, &pps);
+	if (ret) {
+		dev_err(&dsi->dev, "Failed to set PPS\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 static int visionox_vtdr6130_get_modes(struct drm_panel *panel,
 				       struct drm_connector *connector)
 {
@@ -228,6 +260,7 @@ static const struct drm_panel_funcs visionox_vtdr6130_panel_funcs = {
 	.prepare = visionox_vtdr6130_prepare,
 	.unprepare = visionox_vtdr6130_unprepare,
 	.get_modes = visionox_vtdr6130_get_modes,
+	.enable = visionox_vtdr6130_enable,
 };
 
 static int visionox_vtdr6130_bl_update_status(struct backlight_device *bl)
@@ -260,12 +293,32 @@ static int visionox_vtdr6130_probe(struct mipi_dsi_device *dsi)
 {
 	struct device *dev = &dsi->dev;
 	struct visionox_vtdr6130 *ctx;
+	struct drm_dsc_config *dsc;
 	int ret;
 
 	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
 	if (!ctx)
 		return -ENOMEM;
 
+	ctx->cmd_mode = of_property_read_bool(dev->of_node, "enforce-cmd-mode");
+	ctx->dsc_enable = of_property_read_bool(dev->of_node, "enable-dsc");
+
+	/* Set DSC params */
+	if (ctx->dsc_enable) {
+		dsc = &ctx->dsc;
+		dsc->dsc_version_major = 0x1;
+		dsc->dsc_version_minor = 0x2;
+		dsc->slice_height = 40;
+		dsc->slice_width = 540;
+		dsc->slice_count = 2;
+		dsc->slice_per_pkt = 1;
+		dsc->bits_per_component = 8;
+		dsc->bits_per_pixel = 8 << 4;
+		dsc->block_pred_enable = true;
+
+		dsi->dsc = dsc;
+	}
+
 	ctx->supplies[0].supply = "vddio";
 	ctx->supplies[1].supply = "vci";
 	ctx->supplies[2].supply = "vdd";
@@ -306,6 +359,9 @@ static int visionox_vtdr6130_probe(struct mipi_dsi_device *dsi)
 		return ret;
 	}
 
+	dev_err(&dsi->dev, "discovered with %s mode %s\n",
+		ctx->cmd_mode ? "cmd" : "video",
+		ctx->dsc_enable ? "and DSC enabled" : "");
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH 1/3] drm/msm/dpu: fix DSC for DSI video mode

[Dmitry Baryshkov]

On Thu, 28 Mar 2024 at 13:12, Jun Nie <jun.nie@linaro.org> wrote:
>
> Fix DSC timing and control configurations in DPU for DSI video mode.
> Only compression ratio 3:1 is handled and tested.
>
> This patch is modified from patchs of Jonathan Marek.
>
> Signed-off-by: Jun Nie <jun.nie@linaro.org>

This almost looks like a joke, except it isn't the 1st of April yet.
The patch lacks proper Author / Sign-off tags from Jonathan.
This is pretty close to copyright infringement. I'm sorry, but I'd
have to ask you to abstain from sending patches w/o prior internal
review.

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   |  2 +-
>  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  2 +-
>  .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 12 +++++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c   | 10 +++-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h   |  1 +
>  drivers/gpu/drm/msm/dsi/dsi.xml.h             |  1 +
>  drivers/gpu/drm/msm/dsi/dsi_host.c            | 48 +++++++++++--------
>  include/drm/display/drm_dsc.h                 |  4 ++

Ok. The feedback for the original patchset [1]  was that it should be
split logically. Instead you pile everything together into a single
patch. This is a complete no-go.

Also, this patchset lacks changelog in comparison to the previous
patchseris. I don't think I'll continue the review of this patch.
Please rework it properly and add corresponding changelog.

[1] https://patchwork.freedesktop.org/patch/567518/?series=126430&rev=1

>  8 files changed, 56 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 6a4b489d44e5..c1b9da06dde2 100644

-- 
With best wishes
Dmitry

Re: [PATCH 2/3] dt-bindings: display: panel: visionox,vtdr6130: Add mode property

[Dmitry Baryshkov]

On Thu, 28 Mar 2024 at 13:12, Jun Nie <jun.nie@linaro.org> wrote:
>
> Add DSI mode property and compression mode property
>
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  .../bindings/display/panel/visionox,vtdr6130.yaml         | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/panel/visionox,vtdr6130.yaml b/Documentation/devicetree/bindings/display/panel/visionox,vtdr6130.yaml
> index d5a8295106c1..36ae94fa96ec 100644
> --- a/Documentation/devicetree/bindings/display/panel/visionox,vtdr6130.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/visionox,vtdr6130.yaml
> @@ -26,6 +26,14 @@ properties:
>    port: true
>    reset-gpios: true
>
> +  enforce-cmd-mode:
> +    type: boolean
> +    description: Set DSI as command mode. Video mode by default.
> +
> +  enable-dsc:
> +    type: boolean
> +    description: Enable display stream compression
> +

Bindings describe the hardware, so this is a no-go.

>  additionalProperties: false
>
>  required:
> --
> 2.34.1
>


-- 
With best wishes
Dmitry

Re: [PATCH 3/3] drm/panel: Enable DSC for Visionox VTDR6130 panel

[Dmitry Baryshkov]

On Thu, 28 Mar 2024 at 13:12, Jun Nie <jun.nie@linaro.org> wrote:
>
> Enable display compression (DSC v1.2) for 1080x2400 Visionox
> VTDR6130 AMOLED DSI panel. DTS property is needed to enable DSC.
> Default configuration is video mode + non-DSC for any back
> compatibility.

The changelog talks about the DSC only, while the changes also include
the CMD mode support. This should be split to a separate patch.

>
> Below modes works on SM8650. While cmd mode does not work yet.
> - video mode + DSC
> - video mode
> - cmd mode + DSC
>
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  .../gpu/drm/panel/panel-visionox-vtdr6130.c   | 58 ++++++++++++++++++-
>  1 file changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
> index 540099253e1b..d6c44816cdd0 100644
> --- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
> +++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
> @@ -9,6 +9,7 @@
>  #include <linux/of.h>
>
>  #include <drm/display/drm_dsc.h>
> +#include <drm/display/drm_dsc_helper.h>
>  #include <drm/drm_mipi_dsi.h>
>  #include <drm/drm_modes.h>
>  #include <drm/drm_panel.h>
> @@ -17,9 +18,12 @@
>
>  struct visionox_vtdr6130 {
>         struct drm_panel panel;
> +       struct drm_dsc_config dsc;
>         struct mipi_dsi_device *dsi;
>         struct gpio_desc *reset_gpio;
>         struct regulator_bulk_data supplies[3];
> +       bool cmd_mode;
> +       bool dsc_enable;
>  };
>
>  static inline struct visionox_vtdr6130 *to_visionox_vtdr6130(struct drm_panel *panel)
> @@ -49,12 +53,20 @@ static int visionox_vtdr6130_on(struct visionox_vtdr6130 *ctx)
>         if (ret)
>                 return ret;
>
> +       if (ctx->dsc_enable)
> +               mipi_dsi_dcs_write_seq(dsi, 0x03, 0x01);

It it really MIPI_DCS_GET_COMPRESSION_MODE?
> +
>         mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, 0x20);
>         mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS, 0x00, 0x00);
>         mipi_dsi_dcs_write_seq(dsi, 0x59, 0x09);
>         mipi_dsi_dcs_write_seq(dsi, 0x6c, 0x01);
>         mipi_dsi_dcs_write_seq(dsi, 0x6d, 0x00);
> -       mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01);
> +
> +       if (ctx->cmd_mode)
> +               mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x02);
> +       else
> +               mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01);
> +
>         mipi_dsi_dcs_write_seq(dsi, 0x70,
>                                0x12, 0x00, 0x00, 0xab, 0x30, 0x80, 0x09, 0x60, 0x04,
>                                0x38, 0x00, 0x28, 0x02, 0x1c, 0x02, 0x1c, 0x02, 0x00,
> @@ -205,6 +217,26 @@ static const struct drm_display_mode visionox_vtdr6130_mode = {
>         .height_mm = 157,
>  };
>
> +static int visionox_vtdr6130_enable(struct drm_panel *panel)
> +{
> +       struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel);
> +       struct mipi_dsi_device *dsi = ctx->dsi;
> +       struct drm_dsc_picture_parameter_set pps;
> +       int ret;
> +
> +       if (!dsi->dsc)
> +               return 0;
> +
> +       drm_dsc_pps_payload_pack(&pps, dsi->dsc);
> +       ret = mipi_dsi_picture_parameter_set(dsi, &pps);
> +       if (ret) {
> +               dev_err(&dsi->dev, "Failed to set PPS\n");
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
>  static int visionox_vtdr6130_get_modes(struct drm_panel *panel,
>                                        struct drm_connector *connector)
>  {
> @@ -228,6 +260,7 @@ static const struct drm_panel_funcs visionox_vtdr6130_panel_funcs = {
>         .prepare = visionox_vtdr6130_prepare,
>         .unprepare = visionox_vtdr6130_unprepare,
>         .get_modes = visionox_vtdr6130_get_modes,
> +       .enable = visionox_vtdr6130_enable,
>  };
>
>  static int visionox_vtdr6130_bl_update_status(struct backlight_device *bl)
> @@ -260,12 +293,32 @@ static int visionox_vtdr6130_probe(struct mipi_dsi_device *dsi)
>  {
>         struct device *dev = &dsi->dev;
>         struct visionox_vtdr6130 *ctx;
> +       struct drm_dsc_config *dsc;
>         int ret;
>
>         ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>         if (!ctx)
>                 return -ENOMEM;
>
> +       ctx->cmd_mode = of_property_read_bool(dev->of_node, "enforce-cmd-mode");
> +       ctx->dsc_enable = of_property_read_bool(dev->of_node, "enable-dsc");
> +
> +       /* Set DSC params */
> +       if (ctx->dsc_enable) {
> +               dsc = &ctx->dsc;
> +               dsc->dsc_version_major = 0x1;
> +               dsc->dsc_version_minor = 0x2;
> +               dsc->slice_height = 40;
> +               dsc->slice_width = 540;
> +               dsc->slice_count = 2;
> +               dsc->slice_per_pkt = 1;
> +               dsc->bits_per_component = 8;
> +               dsc->bits_per_pixel = 8 << 4;
> +               dsc->block_pred_enable = true;
> +
> +               dsi->dsc = dsc;
> +       }
> +
>         ctx->supplies[0].supply = "vddio";
>         ctx->supplies[1].supply = "vci";
>         ctx->supplies[2].supply = "vdd";
> @@ -306,6 +359,9 @@ static int visionox_vtdr6130_probe(struct mipi_dsi_device *dsi)
>                 return ret;
>         }
>
> +       dev_err(&dsi->dev, "discovered with %s mode %s\n",
> +               ctx->cmd_mode ? "cmd" : "video",
> +               ctx->dsc_enable ? "and DSC enabled" : "");
>         return 0;
>  }
>
> --
> 2.34.1
>


-- 
With best wishes
Dmitry

Re: [PATCH 1/3] drm/msm/dpu: fix DSC for DSI video mode

[Jun Nie]

Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 于2024年3月28日周四 23:05写道：
>
> On Thu, 28 Mar 2024 at 13:12, Jun Nie <jun.nie@linaro.org> wrote:
> >
> > Fix DSC timing and control configurations in DPU for DSI video mode.
> > Only compression ratio 3:1 is handled and tested.
> >
> > This patch is modified from patchs of Jonathan Marek.
> >
> > Signed-off-by: Jun Nie <jun.nie@linaro.org>
>
> This almost looks like a joke, except it isn't the 1st of April yet.
> The patch lacks proper Author / Sign-off tags from Jonathan.
> This is pretty close to copyright infringement. I'm sorry, but I'd
> have to ask you to abstain from sending patches w/o prior internal
> review.

Thanks for pointing me the previous version. I am not aware of it actually.
The only version I knew is from internal repo. It is my fault. I see the slides
says that Jonathan does not want to disturbed, so only his name is
mentioned in the commit message.

What's the patch set status? I do not see it in mainline yet. If it is
in pipeline,
I can just forget the DPU side change.

Thanks!
Jun

>
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   |  2 +-
> >  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  2 +-
> >  .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 12 +++++
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c   | 10 +++-
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h   |  1 +
> >  drivers/gpu/drm/msm/dsi/dsi.xml.h             |  1 +
> >  drivers/gpu/drm/msm/dsi/dsi_host.c            | 48 +++++++++++--------
> >  include/drm/display/drm_dsc.h                 |  4 ++
>
> Ok. The feedback for the original patchset [1]  was that it should be
> split logically. Instead you pile everything together into a single
> patch. This is a complete no-go.
>
> Also, this patchset lacks changelog in comparison to the previous
> patchseris. I don't think I'll continue the review of this patch.
> Please rework it properly and add corresponding changelog.
>
> [1] https://patchwork.freedesktop.org/patch/567518/?series=126430&rev=1
>
> >  8 files changed, 56 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index 6a4b489d44e5..c1b9da06dde2 100644
>
> --
> With best wishes
> Dmitry

Re: [PATCH 1/3] drm/msm/dpu: fix DSC for DSI video mode

[Dmitry Baryshkov]

On Fri, 29 Mar 2024 at 04:47, Jun Nie <jun.nie@linaro.org> wrote:
>
> Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 于2024年3月28日周四 23:05写道：
> >
> > On Thu, 28 Mar 2024 at 13:12, Jun Nie <jun.nie@linaro.org> wrote:
> > >
> > > Fix DSC timing and control configurations in DPU for DSI video mode.
> > > Only compression ratio 3:1 is handled and tested.
> > >
> > > This patch is modified from patchs of Jonathan Marek.
> > >
> > > Signed-off-by: Jun Nie <jun.nie@linaro.org>
> >
> > This almost looks like a joke, except it isn't the 1st of April yet.
> > The patch lacks proper Author / Sign-off tags from Jonathan.
> > This is pretty close to copyright infringement. I'm sorry, but I'd
> > have to ask you to abstain from sending patches w/o prior internal
> > review.
>
> Thanks for pointing me the previous version. I am not aware of it actually.
> The only version I knew is from internal repo. It is my fault. I see the slides
> says that Jonathan does not want to disturbed, so only his name is
> mentioned in the commit message.
>
> What's the patch set status? I do not see it in mainline yet. If it is
> in pipeline,
> I can just forget the DPU side change.

See https://patchwork.freedesktop.org/series/126430/

Jonathan posted the patches, but he didn't seem to be interested in
following up the review feedback.

>
> Thanks!
> Jun
>
> >
> > > ---
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   |  2 +-
> > >  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  2 +-
> > >  .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 12 +++++
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c   | 10 +++-
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h   |  1 +
> > >  drivers/gpu/drm/msm/dsi/dsi.xml.h             |  1 +
> > >  drivers/gpu/drm/msm/dsi/dsi_host.c            | 48 +++++++++++--------
> > >  include/drm/display/drm_dsc.h                 |  4 ++
> >
> > Ok. The feedback for the original patchset [1]  was that it should be
> > split logically. Instead you pile everything together into a single
> > patch. This is a complete no-go.
> >
> > Also, this patchset lacks changelog in comparison to the previous
> > patchseris. I don't think I'll continue the review of this patch.
> > Please rework it properly and add corresponding changelog.
> >
> > [1] https://patchwork.freedesktop.org/patch/567518/?series=126430&rev=1
> >
> > >  8 files changed, 56 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > index 6a4b489d44e5..c1b9da06dde2 100644
> >
> > --
> > With best wishes
> > Dmitry



-- 
With best wishes
Dmitry

Re: [PATCH 2/3] dt-bindings: display: panel: visionox, vtdr6130: Add mode property

[Krzysztof Kozlowski]

On 28/03/2024 12:11, Jun Nie wrote:
> Add DSI mode property and compression mode property
> 
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  .../bindings/display/panel/visionox,vtdr6130.yaml         | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline), work on fork of kernel
(don't, instead use mainline) or you ignore some maintainers (really
don't). Just use b4 and everything should be fine, although remember
about `b4 prep --auto-to-cc` if you added new patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time, thus I will skip this patch entirely till you follow
the process allowing the patch to be tested.

Please kindly resend and include all necessary To/Cc entries.


Best regards,
Krzysztof