Hi,
Thank you for your patch.
On Fri, 28 Jan 2022 at 20:29, Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>
wrote:
Normally, mdp will push one pixel of data per pixel clock to
interface to display. Wide bus feature will increase bus
width from 32 bits to 64 bits so that it can push two
pixel of data per pixel clock to interface to display.
This feature is pre requirement to support 4k resolution.
Signed-off-by: Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 16 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 2 +
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 14 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 108
+++++++++++++++------
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 5 +
drivers/gpu/drm/msm/dp/dp_catalog.c | 11 ++-
drivers/gpu/drm/msm/dp/dp_catalog.h | 1 +
drivers/gpu/drm/msm/dp/dp_ctrl.c | 9 +-
drivers/gpu/drm/msm/dp/dp_ctrl.h | 1 +
drivers/gpu/drm/msm/dp/dp_display.c | 17 ++++
drivers/gpu/drm/msm/dp/dp_display.h | 3 +
drivers/gpu/drm/msm/dp/dp_parser.c | 26 +++++
drivers/gpu/drm/msm/dp/dp_parser.h | 2 +
drivers/gpu/drm/msm/msm_drv.h | 9 ++
Can we get this split into logical chunks please?
14 files changed, 190 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 1e648db..e2fb5bc 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -199,6 +199,8 @@ struct dpu_encoder_virt {
struct msm_display_info disp_info;
+ struct msm_op_info op_info;
+
bool idle_pc_supported;
struct mutex rc_lock;
enum dpu_enc_rc_states rc_state;
@@ -217,6 +219,13 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = {
15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10
};
+bool dpu_encoder_is_widebus_enabled(struct drm_encoder *drm_enc)
+{
+ struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
+
+ return dpu_enc->op_info.wide_bus_en;
+}
+
static void _dpu_encoder_setup_dither(struct dpu_hw_pingpong
*hw_pp, unsigned bpc)
{
struct dpu_hw_dither_cfg dither_cfg = { 0 };
@@ -2112,6 +2121,7 @@ int dpu_encoder_setup(struct drm_device *dev,
struct drm_encoder *enc,
struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms);
struct drm_encoder *drm_enc = NULL;
struct dpu_encoder_virt *dpu_enc = NULL;
+ struct msm_op_info *op_info;
int ret = 0;
dpu_enc = to_dpu_encoder_virt(enc);
@@ -2128,8 +2138,12 @@ int dpu_encoder_setup(struct drm_device *dev,
struct drm_encoder *enc,
timer_setup(&dpu_enc->vsync_event_timer,
dpu_encoder_vsync_event_handler,
0);
- else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
+ else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS) {
dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
+ op_info = &priv->op_info[disp_info->h_tile_instance[0]];
op_info should be defined per INTF rather than per h_tile. This way
you won't have to check for intf_type here.
+ dpu_enc->op_info = *op_info;
So... we set this data in msm_drm_private only to copy it to
dpu_encoder_virt? Please don't do this.
Allow one to query the data from the DP rather than blindly copying it
over and over again.
+
+ }
INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
dpu_encoder_off_work);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index e241914..0d73550 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -168,4 +168,6 @@ int dpu_encoder_get_linecount(struct drm_encoder
*drm_enc);
*/
int dpu_encoder_get_vsync_count(struct drm_encoder *drm_enc);
+bool dpu_encoder_is_widebus_enabled(struct drm_encoder *drm_enc);
+
#endif /* __DPU_ENCODER_H__ */
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 ddd9d89..04ac2dc 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
@@ -110,6 +110,20 @@ static void drm_mode_to_intf_timing_params(
timing->v_back_porch += timing->v_front_porch;
timing->v_front_porch = 0;
}
+
+ timing->wide_bus_en =
dpu_encoder_is_widebus_enabled(phys_enc->parent);
+
+ /*
+ * for DP, divide the horizonal parameters by 2 when
+ * widebus is enabled
+ */
+ if (phys_enc->hw_intf->cap->type == INTF_DP &&
timing->wide_bus_en) {
What about INTF_eDP?
I suspect that intf type check is unnecessary here.
+ timing->width = timing->width >> 1;
+ timing->xres = timing->xres >> 1;
+ timing->h_back_porch = timing->h_back_porch >> 1;
+ timing->h_front_porch = timing->h_front_porch >> 1;
+ timing->hsync_pulse_width = timing->hsync_pulse_width
>> 1;
+ }
}
static u32 get_horizontal_total(const struct 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 116e2b5..f072bd5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -33,6 +33,7 @@
#define INTF_TP_COLOR1 0x05C
#define INTF_CONFIG2 0x060
#define INTF_DISPLAY_DATA_HCTL 0x064
+#define INTF_ACTIVE_DATA_HCTL 0x068
#define INTF_FRAME_LINE_COUNT_EN 0x0A8
#define INTF_FRAME_COUNT 0x0AC
#define INTF_LINE_COUNT 0x0B0
@@ -90,67 +91,109 @@ static void
dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
u32 hsync_period, vsync_period;
u32 display_v_start, display_v_end;
u32 hsync_start_x, hsync_end_x;
+ u32 hsync_data_start_x, hsync_data_end_x;
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 panel_format;
- u32 intf_cfg, intf_cfg2 = 0, display_data_hctl = 0;
+ u32 intf_cfg, intf_cfg2 = 0;
+ u32 display_data_hctl = 0, active_data_hctl = 0;
+ u32 data_width;
+ bool dp_intf = false;
/* read interface_cfg */
intf_cfg = DPU_REG_READ(c, INTF_CONFIG);
+
+ if (ctx->cap->type == INTF_EDP || ctx->cap->type == INTF_DP)
+ dp_intf = true;
+
hsync_period = p->hsync_pulse_width + p->h_back_porch +
p->width +
p->h_front_porch;
vsync_period = p->vsync_pulse_width + p->v_back_porch +
p->height +
p->v_front_porch;
display_v_start = ((p->vsync_pulse_width + p->v_back_porch) *
- hsync_period) + p->hsync_skew;
+ hsync_period) + p->hsync_skew;
Unnecessary whitespace changes complicate reviewing. I'll try groking
this piece of code later.
display_v_end = ((vsync_period - p->v_front_porch) *
hsync_period) +
- p->hsync_skew - 1;
+ p->hsync_skew - 1;
+
+ hsync_ctl = (hsync_period << 16) | p->hsync_pulse_width;
hsync_start_x = p->h_back_porch + p->hsync_pulse_width;
hsync_end_x = hsync_period - p->h_front_porch - 1;
- if (p->width != p->xres) {
- active_h_start = hsync_start_x;
- active_h_end = active_h_start + p->xres - 1;
- } else {
- active_h_start = 0;
- active_h_end = 0;
- }
+ /*
+ * DATA_HCTL_EN controls data timing which can be different from
+ * video timing. It is recommended to enable it for all
cases, except
+ * if compression is enabled in 1 pixel per clock mode
+ */
+ if (!p->compression_en || p->wide_bus_en)
+ intf_cfg2 |= BIT(4);
- if (p->height != p->yres) {
- active_v_start = display_v_start;
- active_v_end = active_v_start + (p->yres *
hsync_period) - 1;
- } else {
- active_v_start = 0;
- active_v_end = 0;
- }
+ if (p->wide_bus_en)
+ intf_cfg2 |= BIT(0);
+
+ /*
+ * If widebus is disabled:
+ * For uncompressed stream, the data is valid for the entire
active
+ * window period.
+ * For compressed stream, data is valid for a shorter time
period
+ * inside the active window depending on the compression ratio.
+ *
+ * If widebus is enabled:
+ * For uncompressed stream, data is valid for only half the
active
+ * window, since the data rate is doubled in this mode.
+ * p->width holds the adjusted width for DP but unadjusted
width for DSI
+ * For compressed stream, data validity window needs to be
adjusted for
+ * compression ratio and then further halved.
+ */
+ data_width = p->width;
+
+ if (p->compression_en) {
+ data_width = DIV_ROUND_UP(p->dce_bytes_per_line, 3);
- if (active_h_end) {
- active_hctl = (active_h_end << 16) | active_h_start;
- intf_cfg |= BIT(29); /* ACTIVE_H_ENABLE */
+ if (p->wide_bus_en)
+ data_width >>= 1;
+ } else if (!dp_intf && p->wide_bus_en) {
+ data_width = p->width >> 1;
} else {
- active_hctl = 0;
+ data_width = p->width;
}
- if (active_v_end)
- intf_cfg |= BIT(30); /* ACTIVE_V_ENABLE */
+ hsync_data_start_x = hsync_start_x;
+ hsync_data_end_x = hsync_start_x + data_width - 1;
- hsync_ctl = (hsync_period << 16) | p->hsync_pulse_width;
display_hctl = (hsync_end_x << 16) | hsync_start_x;
+ display_data_hctl = (hsync_data_end_x << 16) |
hsync_data_start_x;
- if (ctx->cap->type == INTF_EDP || ctx->cap->type == INTF_DP) {
- active_h_start = hsync_start_x;
- active_h_end = active_h_start + p->xres - 1;
- active_v_start = display_v_start;
- active_v_end = active_v_start + (p->yres *
hsync_period) - 1;
-
+ if (dp_intf) {
+ // DP timing adjustment
display_v_start += p->hsync_pulse_width +
p->h_back_porch;
+ display_v_end -= p->h_front_porch;
+ }
- active_hctl = (active_h_end << 16) | active_h_start;
+
+ active_h_start = hsync_start_x;
+ active_h_end = active_h_start + p->xres - 1;
+
+ active_v_start = display_v_start;
+ active_v_end = active_v_start + (p->yres * hsync_period) - 1;
+
+ intf_cfg |= BIT(29); /* ACTIVE_H_ENABLE */
+ intf_cfg |= BIT(30); /* ACTIVE_V_ENABLE */
+
+ active_hctl = (active_h_end << 16) | active_h_start;
+
+ if (dp_intf) {
display_hctl = active_hctl;
+ if (p->compression_en) {
I assume that compression_en is a part of DSC support for the DP,
isn't it?
If so, it should definitely come as a separate patch.
+ active_data_hctl = (hsync_start_x +
+ p->extra_dto_cycles) << 16;
+ active_data_hctl += hsync_start_x;
+
+ display_data_hctl = active_data_hctl;
+ }
}
den_polarity = 0;
@@ -204,6 +247,9 @@ static void
dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *ctx,
DPU_REG_WRITE(c, INTF_FRAME_LINE_COUNT_EN, 0x3);
DPU_REG_WRITE(c, INTF_CONFIG, intf_cfg);
DPU_REG_WRITE(c, INTF_PANEL_FORMAT, panel_format);
+ DPU_REG_WRITE(c, INTF_CONFIG2, intf_cfg2);
+ DPU_REG_WRITE(c, INTF_DISPLAY_DATA_HCTL, display_data_hctl);
+ DPU_REG_WRITE(c, INTF_ACTIVE_DATA_HCTL, active_data_hctl);
Are these registers present on all supported hardware (like sdm845)?
Does msm8998 support them? msm8996?
