diff mbox series

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

Message ID 20240328111158.2074351-1-jun.nie@linaro.org (mailing list archive)
State New, archived
Headers show
Series [1/3] drm/msm/dpu: fix DSC for DSI video mode | expand

Commit Message

Jun Nie March 28, 2024, 11:11 a.m. UTC
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(-)

Comments

Dmitry Baryshkov March 28, 2024, 3:05 p.m. UTC | #1
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
Jun Nie March 29, 2024, 2:47 a.m. UTC | #2
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
Dmitry Baryshkov March 29, 2024, 3:25 a.m. UTC | #3
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
diff mbox series

Patch

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
 	 */