This is follow up update to Jonathan's patch set.
Changes vs V3:
- Rebase to latest msm-next-lumag branch.
- Drop the slice_per_pkt change as it does impact basic DSC feature.
- Remove change in generated dsi header
- update DSC compressed width calculation with bpp and bpc
- split wide bus impact on width into another patch
- rename patch tile of VIDEO_COMPRESSION_MODE_CTRL_WC change
- Polish warning usage
- Add tags from reviewers
Changes vs V2:
- Drop the INTF_CFG2_DATA_HCTL_EN change as it is handled in
latest mainline code.
- Drop the bonded DSI patch as I do not have device to test it.
- Address comments from version 2.
Signed-off-by: Jun Nie <[email protected]>
---
Jonathan Marek (4):
drm/msm/dpu: fix video mode DSC for DSI
drm/msm/dsi: set video mode widebus enable bit when widebus is enabled
drm/msm/dsi: set VIDEO_COMPRESSION_MODE_CTRL_WC
drm/msm/dsi: add a comment to explain pkt_per_line encoding
Jun Nie (1):
drm: adjust data width for widen bus case
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 8 ++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 13 +++++++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 12 ++++++++++++
drivers/gpu/drm/msm/dsi/dsi_host.c | 10 +++++++++-
5 files changed, 43 insertions(+), 2 deletions(-)
---
base-commit: e6428bcb611f6c164856a41fc5a1ae8471a9b5a9
change-id: 20240524-msm-drm-dsc-dsi-video-upstream-4-22e2266fbe89
Best regards,
--
Jun Nie <[email protected]>
From: Jonathan Marek <[email protected]>
The value returned by msm_dsi_wide_bus_enabled() doesn't match what the
driver is doing in video mode. Fix that by actually enabling widebus for
video mode.
Fixes: efcbd6f9cdeb ("drm/msm/dsi: Enable widebus for DSI")
Signed-off-by: Jonathan Marek <[email protected]>
Reviewed-by: Dmitry Baryshkov <[email protected]>
Reviewed-by: Marijn Suijten <[email protected]>
Reviewed-by: Jessica Zhang <[email protected]>
Signed-off-by: Jun Nie <[email protected]>
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index a50f4dda5941..47f5858334f6 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -754,6 +754,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 */
@@ -778,7 +780,6 @@ static void dsi_ctrl_enable(struct msm_dsi_host *msm_host,
if (cfg_hnd->minor >= MSM_DSI_6G_VER_MINOR_V1_3)
data |= DSI_CMD_MODE_MDP_CTRL2_BURST_MODE;
- /* TODO: Allow for video-mode support once tested/fixed */
if (msm_dsi_host_is_wide_bus_enabled(&msm_host->base))
data |= DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN;
--
2.34.1
data is valid for only half the active window if widebus
is enabled
Signed-off-by: Jun Nie <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 8 ++++++++
1 file changed, 8 insertions(+)
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 2cf1f8c116b5..3d1bc8fa4ca2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -167,6 +167,14 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *intf,
intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
data_width = p->width;
+ /*
+ * If widebus is enabled, data is valid for only half the active window
+ * since the data rate is doubled in this mode. But for the compression
+ * mode in DP case, the p->width is already adjusted in
+ * drm_mode_to_intf_timing_params()
+ */
+ if (p->wide_bus_en && !dp_intf)
+ data_width = p->width >> 1;
/* TODO: handle DSC+DP case, we only handle DSC+DSI case so far */
if (p->compression_en && !dp_intf)
--
2.34.1
From: Jonathan Marek <[email protected]>
Make it clear why the pkt_per_line value is being "divided by 2".
Signed-off-by: Jonathan Marek <[email protected]>
Reviewed-by: Dmitry Baryshkov <[email protected]>
Signed-off-by: Jun Nie <[email protected]>
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 7252d36687e6..4768cff08381 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -885,7 +885,11 @@ 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)
*/
+ if (pkt_per_line > 4)
+ drm_warn_once(msm_host->dev, "pkt_per_line too big");
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);
reg |= DSI_VIDEO_COMPRESSION_MODE_CTRL_EN;
--
2.34.1
From: Jonathan Marek <[email protected]>
Video mode DSC won't work if this field is not set correctly. Set it to fix
video mode DSC (for slice_per_pkt==1 cases at least).
Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration")
Signed-off-by: Jonathan Marek <[email protected]>
Reviewed-by: Dmitry Baryshkov <[email protected]>
Signed-off-by: Jun Nie <[email protected]>
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 47f5858334f6..7252d36687e6 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -857,6 +857,7 @@ 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;
/* first calculate dsc parameters and then program
* compress mode registers
@@ -864,6 +865,7 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod
slice_per_intf = msm_dsc_get_slices_per_intf(dsc, hdisplay);
total_bytes_per_intf = dsc->slice_chunk_size * slice_per_intf;
+ bytes_per_pkt = dsc->slice_chunk_size; /* * slice_per_pkt; */
eol_byte_num = total_bytes_per_intf % 3;
@@ -901,6 +903,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);
}
}
--
2.34.1
From: Jonathan Marek <[email protected]>
Add necessary DPU timing and control changes for DSC to work with DSI
video mode.
Signed-off-by: Jonathan Marek <[email protected]>
Signed-off-by: Jun Nie <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 8 ++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 13 +++++++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 ++++
4 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 119f3ea50a7c..48cef6e79c70 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -564,7 +564,7 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc)
return (num_dsc > 0) && (num_dsc > intf_count);
}
-static struct drm_dsc_config *dpu_encoder_get_dsc_config(struct drm_encoder *drm_enc)
+struct drm_dsc_config *dpu_encoder_get_dsc_config(struct drm_encoder *drm_enc)
{
struct msm_drm_private *priv = drm_enc->dev->dev_private;
struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
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 002e89cc1705..2167c46c1a45 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -334,6 +334,14 @@ static inline enum dpu_3d_blend_mode dpu_encoder_helper_get_3d_blend_mode(
*/
unsigned int dpu_encoder_helper_get_dsc(struct dpu_encoder_phys *phys_enc);
+/**
+ * dpu_encoder_get_dsc_config - get DSC config for the DPU encoder
+ * This helper function is used by physical encoder to get DSC config
+ * used for this encoder.
+ * @drm_enc: Pointer to encoder structure
+ */
+struct drm_dsc_config *dpu_encoder_get_dsc_config(struct drm_encoder *drm_enc);
+
/**
* dpu_encoder_get_drm_fmt - return DRM fourcc format
* @phys_enc: Pointer to physical encoder structure
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 ef69c2f408c3..7047b607ca91 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
@@ -115,6 +115,19 @@ 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. bits of 3 components(R/G/B)
+ * is compressed into bits of 1 pixel.
+ */
+ if (phys_enc->hw_intf->cap->type != INTF_DP && timing->compression_en) {
+ struct drm_dsc_config *dsc =
+ dpu_encoder_get_dsc_config(phys_enc->parent);
+ timing->width = timing->width * (dsc->bits_per_pixel >> 4) /
+ (dsc->bits_per_component * 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 225c1c7768ff..2cf1f8c116b5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -168,6 +168,10 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *intf,
data_width = p->width;
+ /* TODO: handle DSC+DP case, we only handle DSC+DSI case so far */
+ if (p->compression_en && !dp_intf)
+ intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
+
hsync_data_start_x = hsync_start_x;
hsync_data_end_x = hsync_start_x + data_width - 1;
--
2.34.1
On Fri, May 24, 2024 at 09:18:21PM +0800, Jun Nie wrote:
> From: Jonathan Marek <[email protected]>
>
> Add necessary DPU timing and control changes for DSC to work with DSI
> video mode.
>
> Signed-off-by: Jonathan Marek <[email protected]>
> Signed-off-by: Jun Nie <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +-
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 8 ++++++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 13 +++++++++++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 4 ++++
> 4 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 119f3ea50a7c..48cef6e79c70 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -564,7 +564,7 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc)
> return (num_dsc > 0) && (num_dsc > intf_count);
> }
>
> -static struct drm_dsc_config *dpu_encoder_get_dsc_config(struct drm_encoder *drm_enc)
> +struct drm_dsc_config *dpu_encoder_get_dsc_config(struct drm_encoder *drm_enc)
> {
> struct msm_drm_private *priv = drm_enc->dev->dev_private;
> struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> 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 002e89cc1705..2167c46c1a45 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> @@ -334,6 +334,14 @@ static inline enum dpu_3d_blend_mode dpu_encoder_helper_get_3d_blend_mode(
> */
> unsigned int dpu_encoder_helper_get_dsc(struct dpu_encoder_phys *phys_enc);
>
> +/**
> + * dpu_encoder_get_dsc_config - get DSC config for the DPU encoder
> + * This helper function is used by physical encoder to get DSC config
> + * used for this encoder.
> + * @drm_enc: Pointer to encoder structure
> + */
> +struct drm_dsc_config *dpu_encoder_get_dsc_config(struct drm_encoder *drm_enc);
> +
> /**
> * dpu_encoder_get_drm_fmt - return DRM fourcc format
> * @phys_enc: Pointer to physical encoder structure
> 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 ef69c2f408c3..7047b607ca91 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
> @@ -115,6 +115,19 @@ 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. bits of 3 components(R/G/B)
> + * is compressed into bits of 1 pixel.
> + */
> + if (phys_enc->hw_intf->cap->type != INTF_DP && timing->compression_en) {
> + struct drm_dsc_config *dsc =
> + dpu_encoder_get_dsc_config(phys_enc->parent);
> + timing->width = timing->width * (dsc->bits_per_pixel >> 4) /
Here you are truncating fractional part of BPP. Please use
drm_dsc_get_bpp_int(), at least it will warn if there is a fractional
part. Or, even better, add a helper to calculate width * bpp / 64 / (bpc
* 3) and use it here and in dsi_adjust_pclk_for_compression()
> + (dsc->bits_per_component * 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 225c1c7768ff..2cf1f8c116b5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -168,6 +168,10 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *intf,
>
> data_width = p->width;
>
> + /* TODO: handle DSC+DP case, we only handle DSC+DSI case so far */
> + if (p->compression_en && !dp_intf)
> + intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
> +
Separate commit, please
> hsync_data_start_x = hsync_start_x;
> hsync_data_end_x = hsync_start_x + data_width - 1;
>
>
> --
> 2.34.1
>
--
With best wishes
Dmitry