DPU 5.x+ and DSI 6G 2.5.x+ support a databus-widen mode that allows for
more compressed data to be transferred per pclk.
This series adds support for enabling this feature for both DPU and DSI
by doing the following:
1. Add a DPU_INTF_DATABUS_WIDEN feature flag
2. Add a DPU INTF op to set the DATABUS_WIDEN register
3. Set the DATABUS_WIDEN register and do the proper hdisplay
calculations in DSI when applicable
Note: This series will only enable the databus-widen mode for command
mode as we are currently unable to validate it on video mode.
Depends on: "Add DSC v1.2 Support for DSI" [1]
[1] https://patchwork.freedesktop.org/series/117219/
Signed-off-by: Jessica Zhang <[email protected]>
---
Jessica Zhang (3):
drm/msm/dpu: Add DPU_INTF_DATABUS_WIDEN feature flag for DPU >= 5.0
drm/msm/dpu: Set DATABUS_WIDEN on command mode encoders
drm/msm/dsi: Enable DATABUS_WIDEN for DSI command mode
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 3 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 12 ++++++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 3 +++
drivers/gpu/drm/msm/dsi/dsi.xml.h | 1 +
drivers/gpu/drm/msm/dsi/dsi_host.c | 19 ++++++++++++++++++-
7 files changed, 41 insertions(+), 2 deletions(-)
---
base-commit: 1981c2c0c05f5d7fe4d4552d4f352cb46840e51e
change-id: 20230525-add-widebus-support-f785546ee751
Best regards,
--
Jessica Zhang <[email protected]>
Add a DPU INTF op to set the DATABUS_WIDEN register to enable the
databus-widen mode datapath.
Signed-off-by: Jessica Zhang <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 3 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 12 ++++++++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 3 +++
3 files changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index b856c6286c85..124ba96bebda 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -70,6 +70,9 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
if (intf_cfg.dsc != 0 && phys_enc->hw_intf->ops.enable_compression)
phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
+
+ if (phys_enc->hw_intf->ops.enable_widebus)
+ phys_enc->hw_intf->ops.enable_widebus(phys_enc->hw_intf);
}
static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int irq_idx)
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 5b0f6627e29b..03ba3a1c7a46 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -513,6 +513,15 @@ static void dpu_hw_intf_disable_autorefresh(struct dpu_hw_intf *intf,
}
+static void dpu_hw_intf_enable_widebus(struct dpu_hw_intf *ctx)
+{
+ u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2);
+
+ intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
+
+ DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
+}
+
static void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
{
u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2);
@@ -545,6 +554,9 @@ static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
if (cap & BIT(DPU_INTF_DATA_COMPRESS))
ops->enable_compression = dpu_hw_intf_enable_compression;
+
+ if (cap & BIT(DPU_INTF_DATABUS_WIDEN))
+ ops->enable_widebus = dpu_hw_intf_enable_widebus;
}
struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
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 99e21c4137f9..64a17b99d3d1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
@@ -71,6 +71,7 @@ struct intf_status {
* Return: 0 on success, -ETIMEDOUT on timeout
* @vsync_sel: Select vsync signal for tear-effect configuration
* @enable_compression: Enable data compression
+ * @enable_widebus: Enable widebus
*/
struct dpu_hw_intf_ops {
void (*setup_timing_gen)(struct dpu_hw_intf *intf,
@@ -109,6 +110,8 @@ struct dpu_hw_intf_ops {
void (*disable_autorefresh)(struct dpu_hw_intf *intf, uint32_t encoder_id, u16 vdisplay);
void (*enable_compression)(struct dpu_hw_intf *intf);
+
+ void (*enable_widebus)(struct dpu_hw_intf *intf);
};
struct dpu_hw_intf {
--
2.40.1
DPU 5.x+ supports a databus widen mode that allows more data to be sent
per pclk. Enable this feature flag on all relevant chipsets.
Signed-off-by: Jessica Zhang <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
2 files changed, 4 insertions(+), 1 deletion(-)
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 36ba3f58dcdf..0be7bf0bfc41 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -103,7 +103,8 @@
(BIT(DPU_INTF_INPUT_CTRL) | \
BIT(DPU_INTF_TE) | \
BIT(DPU_INTF_STATUS_SUPPORTED) | \
- BIT(DPU_DATA_HCTL_EN))
+ BIT(DPU_DATA_HCTL_EN) | \
+ BIT(DPU_INTF_DATABUS_WIDEN))
#define INTF_SC7280_MASK (INTF_SC7180_MASK | BIT(DPU_INTF_DATA_COMPRESS))
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
index b860784ade72..b9939e00f5e0 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -182,6 +182,7 @@ enum {
* than video timing
* @DPU_INTF_STATUS_SUPPORTED INTF block has INTF_STATUS register
* @DPU_INTF_DATA_COMPRESS INTF block has DATA_COMPRESS register
+ * @DPU_INTF_DATABUS_WIDEN INTF block has DATABUS_WIDEN register
* @DPU_INTF_MAX
*/
enum {
@@ -190,6 +191,7 @@ enum {
DPU_DATA_HCTL_EN,
DPU_INTF_STATUS_SUPPORTED,
DPU_INTF_DATA_COMPRESS,
+ DPU_INTF_DATABUS_WIDEN,
DPU_INTF_MAX
};
--
2.40.1
DSI 6G v2.5.x+ supports a data-bus widen mode that allows DSI to send
48 bits of compressed data per pclk instead of 24.
For all chipsets that support this mode, enable it whenever DSC is
enabled as recommend by the hardware programming guide.
Only enable this for command mode as we are currently unable to validate
it for video mode.
Signed-off-by: Jessica Zhang <[email protected]>
---
Note: The dsi.xml.h changes were generated using the headergen2 script in
envytools [1], but the changes to the copyright and rules-ng-ng source file
paths were dropped.
[1] https://github.com/freedreno/envytools/
drivers/gpu/drm/msm/dsi/dsi.xml.h | 1 +
drivers/gpu/drm/msm/dsi/dsi_host.c | 19 ++++++++++++++++++-
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h b/drivers/gpu/drm/msm/dsi/dsi.xml.h
index a4a154601114..2a7d980e12c3 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
@@ -664,6 +664,7 @@ static inline uint32_t DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP(enum dsi_rgb_swap v
return ((val) << DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__SHIFT) & DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__MASK;
}
#define DSI_CMD_MODE_MDP_CTRL2_BURST_MODE 0x00010000
+#define DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN 0x00100000
#define REG_DSI_CMD_MODE_MDP_STREAM2_CTRL 0x000001b8
#define DSI_CMD_MODE_MDP_STREAM2_CTRL_DATA_TYPE__MASK 0x0000003f
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 5d7b4409e4e9..1da5238e7105 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -927,6 +927,9 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
u32 hdisplay = mode->hdisplay;
u32 wc;
int ret;
+ bool widebus_supported = msm_host->cfg_hnd->major == MSM_DSI_VER_MAJOR_6G &&
+ msm_host->cfg_hnd->minor >= MSM_DSI_6G_VER_MINOR_V2_5_0;
+
DBG("");
@@ -973,8 +976,15 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
*
* hdisplay will be divided by 3 here to account for the fact
* that DPU sends 3 bytes per pclk cycle to DSI.
+ *
+ * If widebus is supported, set DATABUS_WIDEN register and divide hdisplay by 6
+ * instead of 3
*/
- hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
+ if (!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) && widebus_supported)
+ hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 6);
+ else
+ hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
+
h_total += hdisplay;
ha_end = ha_start + hdisplay;
}
@@ -1027,6 +1037,13 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_TOTAL,
DSI_CMD_MDP_STREAM0_TOTAL_H_TOTAL(hdisplay) |
DSI_CMD_MDP_STREAM0_TOTAL_V_TOTAL(mode->vdisplay));
+
+ if (msm_host->dsc && widebus_supported) {
+ u32 mdp_ctrl2 = dsi_read(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2);
+
+ mdp_ctrl2 |= DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN;
+ dsi_write(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2, mdp_ctrl2);
+ }
}
}
--
2.40.1
On 14/06/2023 04:57, Jessica Zhang wrote:
> Add a DPU INTF op to set the DATABUS_WIDEN register to enable the
> databus-widen mode datapath.
>
> Signed-off-by: Jessica Zhang <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 3 +++
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 12 ++++++++++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 3 +++
> 3 files changed, 18 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> index b856c6286c85..124ba96bebda 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> @@ -70,6 +70,9 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>
> if (intf_cfg.dsc != 0 && phys_enc->hw_intf->ops.enable_compression)
> phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
> +
> + if (phys_enc->hw_intf->ops.enable_widebus)
> + phys_enc->hw_intf->ops.enable_widebus(phys_enc->hw_intf);
No. Please provide a single function which takes necessary
configuration, including compression and wide_bus_enable.
Also note, that we already have dpu_encoder_is_widebus_enabled() and the
rest of support code. Please stick to it too.
> }
>
> static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int irq_idx)
> 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 5b0f6627e29b..03ba3a1c7a46 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -513,6 +513,15 @@ static void dpu_hw_intf_disable_autorefresh(struct dpu_hw_intf *intf,
>
> }
>
> +static void dpu_hw_intf_enable_widebus(struct dpu_hw_intf *ctx)
> +{
> + u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2);
> +
> + intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
> +
> + DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
> +}
> +
> static void dpu_hw_intf_enable_compression(struct dpu_hw_intf *ctx)
> {
> u32 intf_cfg2 = DPU_REG_READ(&ctx->hw, INTF_CONFIG2);
> @@ -545,6 +554,9 @@ static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
>
> if (cap & BIT(DPU_INTF_DATA_COMPRESS))
> ops->enable_compression = dpu_hw_intf_enable_compression;
> +
> + if (cap & BIT(DPU_INTF_DATABUS_WIDEN))
> + ops->enable_widebus = dpu_hw_intf_enable_widebus;
> }
>
> struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
> 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 99e21c4137f9..64a17b99d3d1 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> @@ -71,6 +71,7 @@ struct intf_status {
> * Return: 0 on success, -ETIMEDOUT on timeout
> * @vsync_sel: Select vsync signal for tear-effect configuration
> * @enable_compression: Enable data compression
> + * @enable_widebus: Enable widebus
> */
> struct dpu_hw_intf_ops {
> void (*setup_timing_gen)(struct dpu_hw_intf *intf,
> @@ -109,6 +110,8 @@ struct dpu_hw_intf_ops {
> void (*disable_autorefresh)(struct dpu_hw_intf *intf, uint32_t encoder_id, u16 vdisplay);
>
> void (*enable_compression)(struct dpu_hw_intf *intf);
> +
> + void (*enable_widebus)(struct dpu_hw_intf *intf);
> };
>
> struct dpu_hw_intf {
>
--
With best wishes
Dmitry
On 14/06/2023 04:57, Jessica Zhang wrote:
> DSI 6G v2.5.x+ supports a data-bus widen mode that allows DSI to send
> 48 bits of compressed data per pclk instead of 24.
>
> For all chipsets that support this mode, enable it whenever DSC is
> enabled as recommend by the hardware programming guide.
>
> Only enable this for command mode as we are currently unable to validate
> it for video mode.
>
> Signed-off-by: Jessica Zhang <[email protected]>
> ---
>
> Note: The dsi.xml.h changes were generated using the headergen2 script in
> envytools [1], but the changes to the copyright and rules-ng-ng source file
> paths were dropped.
>
> [1] https://github.com/freedreno/envytools/
>
> drivers/gpu/drm/msm/dsi/dsi.xml.h | 1 +
> drivers/gpu/drm/msm/dsi/dsi_host.c | 19 ++++++++++++++++++-
> 2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> index a4a154601114..2a7d980e12c3 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> @@ -664,6 +664,7 @@ static inline uint32_t DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP(enum dsi_rgb_swap v
> return ((val) << DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__SHIFT) & DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__MASK;
> }
> #define DSI_CMD_MODE_MDP_CTRL2_BURST_MODE 0x00010000
> +#define DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN 0x00100000
>
> #define REG_DSI_CMD_MODE_MDP_STREAM2_CTRL 0x000001b8
> #define DSI_CMD_MODE_MDP_STREAM2_CTRL_DATA_TYPE__MASK 0x0000003f
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 5d7b4409e4e9..1da5238e7105 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -927,6 +927,9 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> u32 hdisplay = mode->hdisplay;
> u32 wc;
> int ret;
> + bool widebus_supported = msm_host->cfg_hnd->major == MSM_DSI_VER_MAJOR_6G &&
> + msm_host->cfg_hnd->minor >= MSM_DSI_6G_VER_MINOR_V2_5_0;
> +
>
> DBG("");
>
> @@ -973,8 +976,15 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> *
> * hdisplay will be divided by 3 here to account for the fact
> * that DPU sends 3 bytes per pclk cycle to DSI.
> + *
> + * If widebus is supported, set DATABUS_WIDEN register and divide hdisplay by 6
> + * instead of 3
This is useless, it is already obvious from the code below. Instead
there should be something like "wide bus extends that to 6 bytes per
pclk cycle"
> */
> - hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
> + if (!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) && widebus_supported)
> + hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 6);
> + else
> + hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
> +
> h_total += hdisplay;
> ha_end = ha_start + hdisplay;
> }
> @@ -1027,6 +1037,13 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_TOTAL,
> DSI_CMD_MDP_STREAM0_TOTAL_H_TOTAL(hdisplay) |
> DSI_CMD_MDP_STREAM0_TOTAL_V_TOTAL(mode->vdisplay));
> +
> + if (msm_host->dsc && widebus_supported) {
> + u32 mdp_ctrl2 = dsi_read(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2);
> +
> + mdp_ctrl2 |= DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN;
> + dsi_write(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2, mdp_ctrl2);
Is widebus applicable only to the CMD mode, or video mode can employ it too?
> + }
> }
> }
>
>
> --
> 2.40.1
>
--
With best wishes
Dmitry
On 14/06/2023 04:57, Jessica Zhang wrote:
> DPU 5.x+ supports a databus widen mode that allows more data to be sent
> per pclk. Enable this feature flag on all relevant chipsets.
>
> Signed-off-by: Jessica Zhang <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 ++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
> 2 files changed, 4 insertions(+), 1 deletion(-)
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On 2023-06-13 18:57:13, Jessica Zhang wrote:
> DSI 6G v2.5.x+ supports a data-bus widen mode that allows DSI to send
> 48 bits of compressed data per pclk instead of 24.
>
> For all chipsets that support this mode, enable it whenever DSC is
> enabled as recommend by the hardware programming guide.
>
> Only enable this for command mode as we are currently unable to validate
> it for video mode.
>
> Signed-off-by: Jessica Zhang <[email protected]>
> ---
>
> Note: The dsi.xml.h changes were generated using the headergen2 script in
> envytools [1], but the changes to the copyright and rules-ng-ng source file
> paths were dropped.
>
> [1] https://github.com/freedreno/envytools/
More interesting would be a link to the Mesa MR upstreaming this
bitfield to dsi.xml [2] (which I have not found on my own yet).
[2]: https://gitlab.freedesktop.org/mesa/mesa/-/blame/main/src/freedreno/registers/dsi/dsi.xml
> drivers/gpu/drm/msm/dsi/dsi.xml.h | 1 +
> drivers/gpu/drm/msm/dsi/dsi_host.c | 19 ++++++++++++++++++-
> 2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> index a4a154601114..2a7d980e12c3 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> @@ -664,6 +664,7 @@ static inline uint32_t DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP(enum dsi_rgb_swap v
> return ((val) << DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__SHIFT) & DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__MASK;
> }
> #define DSI_CMD_MODE_MDP_CTRL2_BURST_MODE 0x00010000
> +#define DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN 0x00100000
>
> #define REG_DSI_CMD_MODE_MDP_STREAM2_CTRL 0x000001b8
> #define DSI_CMD_MODE_MDP_STREAM2_CTRL_DATA_TYPE__MASK 0x0000003f
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 5d7b4409e4e9..1da5238e7105 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -927,6 +927,9 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> u32 hdisplay = mode->hdisplay;
> u32 wc;
> int ret;
> + bool widebus_supported = msm_host->cfg_hnd->major == MSM_DSI_VER_MAJOR_6G &&
> + msm_host->cfg_hnd->minor >= MSM_DSI_6G_VER_MINOR_V2_5_0;
> +
>
> DBG("");
>
> @@ -973,8 +976,15 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> *
> * hdisplay will be divided by 3 here to account for the fact
> * that DPU sends 3 bytes per pclk cycle to DSI.
> + *
> + * If widebus is supported, set DATABUS_WIDEN register and divide hdisplay by 6
> + * instead of 3
So this should allow us to divide pclk by 2, or have much lower latency?
Otherwise it'll tick enough times to transmit the data twice.
Note that I brought up the exact same concerns when you used the 3:1
ratio from dsi_bpp / dsc_bpp in your pclk reduction patch (instad of the
number of bits/bytes that DPU sends to DSI per pclk), but no-one has
replied to my inquiry yet.
> */
> - hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
> + if (!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) && widebus_supported)
> + hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 6);
> + else
> + hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
Nit: I wonder if this is more concise when written as:
u32 bytes_per_pclk;
...
if (!video && widebus)
bytes_per_pclk = 6;
else
bytes_per_pclk = 3;
hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc),
bytes_per_pclk);
That is less duplication, **and** the value becomes self-documenting!
> +
> h_total += hdisplay;
> ha_end = ha_start + hdisplay;
> }
> @@ -1027,6 +1037,13 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_TOTAL,
> DSI_CMD_MDP_STREAM0_TOTAL_H_TOTAL(hdisplay) |
> DSI_CMD_MDP_STREAM0_TOTAL_V_TOTAL(mode->vdisplay));
> +
> + if (msm_host->dsc && widebus_supported) {
Can we also support widebus for uncompressed streams (sending 2 pixels
of bpp=24 per pclk), and if so, is that something you want to add in the
future (a comment would be nice)?
> + u32 mdp_ctrl2 = dsi_read(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2);
> +
> + mdp_ctrl2 |= DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN;
> + dsi_write(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2, mdp_ctrl2);
> + }
Same comment as on your BURST_MODE patch (which this'll conflict with):
does this belong to the timing setup or is it better moved to
dsi_ctrl_config?
- Marijn
> }
> }
>
>
> --
> 2.40.1
>
On 2023-06-13 18:57:10, Jessica Zhang wrote:
> DPU 5.x+ and DSI 6G 2.5.x+ support a databus-widen mode that allows for
> more compressed data to be transferred per pclk.
>
> This series adds support for enabling this feature for both DPU and DSI
> by doing the following:
>
> 1. Add a DPU_INTF_DATABUS_WIDEN feature flag
> 2. Add a DPU INTF op to set the DATABUS_WIDEN register
> 3. Set the DATABUS_WIDEN register and do the proper hdisplay
> calculations in DSI when applicable
>
> Note: This series will only enable the databus-widen mode for command
> mode as we are currently unable to validate it on video mode.
>
> Depends on: "Add DSC v1.2 Support for DSI" [1]
>
> [1] https://patchwork.freedesktop.org/series/117219/
Didn't Dmitry already pick that up for msm-next-lumag?
- Marijn
> Signed-off-by: Jessica Zhang <[email protected]>
> ---
> Jessica Zhang (3):
> drm/msm/dpu: Add DPU_INTF_DATABUS_WIDEN feature flag for DPU >= 5.0
> drm/msm/dpu: Set DATABUS_WIDEN on command mode encoders
> drm/msm/dsi: Enable DATABUS_WIDEN for DSI command mode
>
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 3 +++
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 ++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 12 ++++++++++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 3 +++
> drivers/gpu/drm/msm/dsi/dsi.xml.h | 1 +
> drivers/gpu/drm/msm/dsi/dsi_host.c | 19 ++++++++++++++++++-
> 7 files changed, 41 insertions(+), 2 deletions(-)
> ---
> base-commit: 1981c2c0c05f5d7fe4d4552d4f352cb46840e51e
> change-id: 20230525-add-widebus-support-f785546ee751
>
> Best regards,
> --
> Jessica Zhang <[email protected]>
>
On 2023-06-14 10:49:31, Dmitry Baryshkov wrote:
> On 14/06/2023 04:57, Jessica Zhang wrote:
> > DSI 6G v2.5.x+ supports a data-bus widen mode that allows DSI to send
> > 48 bits of compressed data per pclk instead of 24.
> >
> > For all chipsets that support this mode, enable it whenever DSC is
> > enabled as recommend by the hardware programming guide.
> >
> > Only enable this for command mode as we are currently unable to validate
> > it for video mode.
> >
> > Signed-off-by: Jessica Zhang <[email protected]>
> > ---
> >
> > Note: The dsi.xml.h changes were generated using the headergen2 script in
> > envytools [1], but the changes to the copyright and rules-ng-ng source file
> > paths were dropped.
> >
> > [1] https://github.com/freedreno/envytools/
> >
> > drivers/gpu/drm/msm/dsi/dsi.xml.h | 1 +
> > drivers/gpu/drm/msm/dsi/dsi_host.c | 19 ++++++++++++++++++-
> > 2 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> > index a4a154601114..2a7d980e12c3 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
> > +++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
> > @@ -664,6 +664,7 @@ static inline uint32_t DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP(enum dsi_rgb_swap v
> > return ((val) << DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__SHIFT) & DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__MASK;
> > }
> > #define DSI_CMD_MODE_MDP_CTRL2_BURST_MODE 0x00010000
> > +#define DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN 0x00100000
> >
> > #define REG_DSI_CMD_MODE_MDP_STREAM2_CTRL 0x000001b8
> > #define DSI_CMD_MODE_MDP_STREAM2_CTRL_DATA_TYPE__MASK 0x0000003f
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > index 5d7b4409e4e9..1da5238e7105 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > @@ -927,6 +927,9 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> > u32 hdisplay = mode->hdisplay;
> > u32 wc;
> > int ret;
> > + bool widebus_supported = msm_host->cfg_hnd->major == MSM_DSI_VER_MAJOR_6G &&
> > + msm_host->cfg_hnd->minor >= MSM_DSI_6G_VER_MINOR_V2_5_0;
> > +
> >
> > DBG("");
> >
> > @@ -973,8 +976,15 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> > *
> > * hdisplay will be divided by 3 here to account for the fact
> > * that DPU sends 3 bytes per pclk cycle to DSI.
> > + *
> > + * If widebus is supported, set DATABUS_WIDEN register and divide hdisplay by 6
> > + * instead of 3
>
> This is useless, it is already obvious from the code below. Instead
> there should be something like "wide bus extends that to 6 bytes per
> pclk cycle"
Yes please. In general, don't paraphrase the code, but explain _why_ it
is doing what it does. Saying that the widebus feature doubles the
bandwidth per pclk tick is much more clear than "divide by 6 instead of
3" - we can read that from the code.
Overall comments have been very good so far (such as the original "to
account for the fact that DPU sends 3 bytes per pclk cycle"), though!
> > */
> > - hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
> > + if (!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) && widebus_supported)
> > + hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 6);
> > + else
> > + hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
> > +
> > h_total += hdisplay;
> > ha_end = ha_start + hdisplay;
> > }
> > @@ -1027,6 +1037,13 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> > dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_TOTAL,
> > DSI_CMD_MDP_STREAM0_TOTAL_H_TOTAL(hdisplay) |
> > DSI_CMD_MDP_STREAM0_TOTAL_V_TOTAL(mode->vdisplay));
> > +
> > + if (msm_host->dsc && widebus_supported) {
> > + u32 mdp_ctrl2 = dsi_read(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2);
> > +
> > + mdp_ctrl2 |= DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN;
> > + dsi_write(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2, mdp_ctrl2);
>
> Is widebus applicable only to the CMD mode, or video mode can employ it too?
The patch description states that it was not tested on video-mode yet,
so I assume it will. But this should also be highlighted with a comment
(e.g. /* XXX: Allow for video-mode once tested/fixed */), _especially_
on the check for MIPI_DSI_MODE_VIDEO above.
If I understand this correctly DSC is not working for video mode at all
on these setups, right? Or no-one was able to test it? I'm inclined to
request dropping these artifical guards to have as little friction as
possible when someone starts enabling and testing this - and less
patches removing artificial bounds in the future.
- Marijn
On 2023-06-13 18:57:11, Jessica Zhang wrote:
> DPU 5.x+ supports a databus widen mode that allows more data to be sent
> per pclk. Enable this feature flag on all relevant chipsets.
>
> Signed-off-by: Jessica Zhang <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 ++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> 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 36ba3f58dcdf..0be7bf0bfc41 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> @@ -103,7 +103,8 @@
> (BIT(DPU_INTF_INPUT_CTRL) | \
> BIT(DPU_INTF_TE) | \
> BIT(DPU_INTF_STATUS_SUPPORTED) | \
> - BIT(DPU_DATA_HCTL_EN))
> + BIT(DPU_DATA_HCTL_EN) | \
> + BIT(DPU_INTF_DATABUS_WIDEN))
This doesn't work. DPU 5.0.0 is SM8150, which has DSI 6G 2.3. In the
last patch for DSI you state and enable widebus for DSI 6G 2.5+ only,
meaning DPU and DSI are now desynced, and the output is completely
corrupted.
Is the bound in dsi_host wrong, or do DPU and DSI need to communicate
when widebus will be enabled, based on DPU && DSI supporting it?
- Marijn
> #define INTF_SC7280_MASK (INTF_SC7180_MASK | BIT(DPU_INTF_DATA_COMPRESS))
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> index b860784ade72..b9939e00f5e0 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> @@ -182,6 +182,7 @@ enum {
> * than video timing
> * @DPU_INTF_STATUS_SUPPORTED INTF block has INTF_STATUS register
> * @DPU_INTF_DATA_COMPRESS INTF block has DATA_COMPRESS register
> + * @DPU_INTF_DATABUS_WIDEN INTF block has DATABUS_WIDEN register
> * @DPU_INTF_MAX
> */
> enum {
> @@ -190,6 +191,7 @@ enum {
> DPU_DATA_HCTL_EN,
> DPU_INTF_STATUS_SUPPORTED,
> DPU_INTF_DATA_COMPRESS,
> + DPU_INTF_DATABUS_WIDEN,
> DPU_INTF_MAX
> };
>
>
> --
> 2.40.1
>
On 14/06/2023 14:42, Marijn Suijten wrote:
> On 2023-06-13 18:57:11, Jessica Zhang wrote:
>> DPU 5.x+ supports a databus widen mode that allows more data to be sent
>> per pclk. Enable this feature flag on all relevant chipsets.
>>
>> Signed-off-by: Jessica Zhang <[email protected]>
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 ++-
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
>> 2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> 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 36ba3f58dcdf..0be7bf0bfc41 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>> @@ -103,7 +103,8 @@
>> (BIT(DPU_INTF_INPUT_CTRL) | \
>> BIT(DPU_INTF_TE) | \
>> BIT(DPU_INTF_STATUS_SUPPORTED) | \
>> - BIT(DPU_DATA_HCTL_EN))
>> + BIT(DPU_DATA_HCTL_EN) | \
>> + BIT(DPU_INTF_DATABUS_WIDEN))
>
> This doesn't work. DPU 5.0.0 is SM8150, which has DSI 6G 2.3. In the
> last patch for DSI you state and enable widebus for DSI 6G 2.5+ only,
> meaning DPU and DSI are now desynced, and the output is completely
> corrupted.
>
> Is the bound in dsi_host wrong, or do DPU and DSI need to communicate
> when widebus will be enabled, based on DPU && DSI supporting it?
I'd prefer to follow the second approach, as we did for DP. DPU asks the
actual video output driver if widebus is to be enabled.
>
> - Marijn
>
>> #define INTF_SC7280_MASK (INTF_SC7180_MASK | BIT(DPU_INTF_DATA_COMPRESS))
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> index b860784ade72..b9939e00f5e0 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> @@ -182,6 +182,7 @@ enum {
>> * than video timing
>> * @DPU_INTF_STATUS_SUPPORTED INTF block has INTF_STATUS register
>> * @DPU_INTF_DATA_COMPRESS INTF block has DATA_COMPRESS register
>> + * @DPU_INTF_DATABUS_WIDEN INTF block has DATABUS_WIDEN register
>> * @DPU_INTF_MAX
>> */
>> enum {
>> @@ -190,6 +191,7 @@ enum {
>> DPU_DATA_HCTL_EN,
>> DPU_INTF_STATUS_SUPPORTED,
>> DPU_INTF_DATA_COMPRESS,
>> + DPU_INTF_DATABUS_WIDEN,
>> DPU_INTF_MAX
>> };
>>
>>
>> --
>> 2.40.1
>>
--
With best wishes
Dmitry
On 2023-06-14 15:01:59, Dmitry Baryshkov wrote:
> On 14/06/2023 14:42, Marijn Suijten wrote:
> > On 2023-06-13 18:57:11, Jessica Zhang wrote:
> >> DPU 5.x+ supports a databus widen mode that allows more data to be sent
> >> per pclk. Enable this feature flag on all relevant chipsets.
> >>
> >> Signed-off-by: Jessica Zhang <[email protected]>
> >> ---
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 ++-
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
> >> 2 files changed, 4 insertions(+), 1 deletion(-)
> >>
> >> 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 36ba3f58dcdf..0be7bf0bfc41 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >> @@ -103,7 +103,8 @@
> >> (BIT(DPU_INTF_INPUT_CTRL) | \
> >> BIT(DPU_INTF_TE) | \
> >> BIT(DPU_INTF_STATUS_SUPPORTED) | \
> >> - BIT(DPU_DATA_HCTL_EN))
> >> + BIT(DPU_DATA_HCTL_EN) | \
> >> + BIT(DPU_INTF_DATABUS_WIDEN))
> >
> > This doesn't work. DPU 5.0.0 is SM8150, which has DSI 6G 2.3. In the
> > last patch for DSI you state and enable widebus for DSI 6G 2.5+ only,
> > meaning DPU and DSI are now desynced, and the output is completely
> > corrupted.
Tested this on SM8350 which actually has DSI 2.5, and it is also
corrupted with this series so something else on this series might be
broken.
> > Is the bound in dsi_host wrong, or do DPU and DSI need to communicate
> > when widebus will be enabled, based on DPU && DSI supporting it?
>
> I'd prefer to follow the second approach, as we did for DP. DPU asks the
> actual video output driver if widebus is to be enabled.
Doesn't it seem very strange that DPU 5.x+ comes with a widebus feature,
but the DSI does not until two revisions later? Or is this available on
every interface, but only for a different (probably DP) encoder block?
- Marijn
On 6/14/2023 5:23 AM, Marijn Suijten wrote:
> On 2023-06-14 15:01:59, Dmitry Baryshkov wrote:
>> On 14/06/2023 14:42, Marijn Suijten wrote:
>>> On 2023-06-13 18:57:11, Jessica Zhang wrote:
>>>> DPU 5.x+ supports a databus widen mode that allows more data to be sent
>>>> per pclk. Enable this feature flag on all relevant chipsets.
>>>>
>>>> Signed-off-by: Jessica Zhang <[email protected]>
>>>> ---
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 ++-
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
>>>> 2 files changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> 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 36ba3f58dcdf..0be7bf0bfc41 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>> @@ -103,7 +103,8 @@
>>>> (BIT(DPU_INTF_INPUT_CTRL) | \
>>>> BIT(DPU_INTF_TE) | \
>>>> BIT(DPU_INTF_STATUS_SUPPORTED) | \
>>>> - BIT(DPU_DATA_HCTL_EN))
>>>> + BIT(DPU_DATA_HCTL_EN) | \
>>>> + BIT(DPU_INTF_DATABUS_WIDEN))
>>>
>>> This doesn't work. DPU 5.0.0 is SM8150, which has DSI 6G 2.3. In the
>>> last patch for DSI you state and enable widebus for DSI 6G 2.5+ only,
>>> meaning DPU and DSI are now desynced, and the output is completely
>>> corrupted.
>
> Tested this on SM8350 which actually has DSI 2.5, and it is also
> corrupted with this series so something else on this series might be
> broken.
>
>>> Is the bound in dsi_host wrong, or do DPU and DSI need to communicate
>>> when widebus will be enabled, based on DPU && DSI supporting it?
>>
>> I'd prefer to follow the second approach, as we did for DP. DPU asks the
>> actual video output driver if widebus is to be enabled.
>
I was afraid of this. This series was made on an assumption that the DPU
version of widebus and DSI version of widebus would be compatible but
looks like already SM8150 is an outlier.
Yes, I think we have to go with second approach.
DPU queries DSI if it supports widebus and enables it.
Thanks for your responses. We will post a v2.
> Doesn't it seem very strange that DPU 5.x+ comes with a widebus feature,
> but the DSI does not until two revisions later? Or is this available on
> every interface, but only for a different (probably DP) encoder block?
>
Yes its strange.
> - Marijn
On 6/14/2023 12:35 PM, Abhinav Kumar wrote:
>
>
> On 6/14/2023 5:23 AM, Marijn Suijten wrote:
>> On 2023-06-14 15:01:59, Dmitry Baryshkov wrote:
>>> On 14/06/2023 14:42, Marijn Suijten wrote:
>>>> On 2023-06-13 18:57:11, Jessica Zhang wrote:
>>>>> DPU 5.x+ supports a databus widen mode that allows more data to be
>>>>> sent
>>>>> per pclk. Enable this feature flag on all relevant chipsets.
>>>>>
>>>>> Signed-off-by: Jessica Zhang <[email protected]>
>>>>> ---
>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 ++-
>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
>>>>> 2 files changed, 4 insertions(+), 1 deletion(-)
>>>>>
>>>>> 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 36ba3f58dcdf..0be7bf0bfc41 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>>> @@ -103,7 +103,8 @@
>>>>> (BIT(DPU_INTF_INPUT_CTRL) | \
>>>>> BIT(DPU_INTF_TE) | \
>>>>> BIT(DPU_INTF_STATUS_SUPPORTED) | \
>>>>> - BIT(DPU_DATA_HCTL_EN))
>>>>> + BIT(DPU_DATA_HCTL_EN) | \
>>>>> + BIT(DPU_INTF_DATABUS_WIDEN))
>>>>
>>>> This doesn't work. DPU 5.0.0 is SM8150, which has DSI 6G 2.3. In the
>>>> last patch for DSI you state and enable widebus for DSI 6G 2.5+ only,
>>>> meaning DPU and DSI are now desynced, and the output is completely
>>>> corrupted.
>>
>> Tested this on SM8350 which actually has DSI 2.5, and it is also
>> corrupted with this series so something else on this series might be
>> broken.
>>
Missed this response. That seems strange.
This series was tested on SM8350 HDK with a command mode panel.
We will fix the DPU-DSI handshake and post a v2 but your issue needs
investigation in parallel.
So another bug to track that would be great.
>>>> Is the bound in dsi_host wrong, or do DPU and DSI need to communicate
>>>> when widebus will be enabled, based on DPU && DSI supporting it?
>>>
>>> I'd prefer to follow the second approach, as we did for DP. DPU asks the
>>> actual video output driver if widebus is to be enabled.
>>
>
> I was afraid of this. This series was made on an assumption that the DPU
> version of widebus and DSI version of widebus would be compatible but
> looks like already SM8150 is an outlier.
>
> Yes, I think we have to go with second approach.
>
> DPU queries DSI if it supports widebus and enables it.
>
> Thanks for your responses. We will post a v2.
>
>> Doesn't it seem very strange that DPU 5.x+ comes with a widebus feature,
>> but the DSI does not until two revisions later? Or is this available on
>> every interface, but only for a different (probably DP) encoder block?
>>
>
> Yes its strange.
>
>> - Marijn
On 6/14/2023 12:54 PM, Abhinav Kumar wrote:
>
>
> On 6/14/2023 12:35 PM, Abhinav Kumar wrote:
>>
>>
>> On 6/14/2023 5:23 AM, Marijn Suijten wrote:
>>> On 2023-06-14 15:01:59, Dmitry Baryshkov wrote:
>>>> On 14/06/2023 14:42, Marijn Suijten wrote:
>>>>> On 2023-06-13 18:57:11, Jessica Zhang wrote:
>>>>>> DPU 5.x+ supports a databus widen mode that allows more data to be
>>>>>> sent
>>>>>> per pclk. Enable this feature flag on all relevant chipsets.
>>>>>>
>>>>>> Signed-off-by: Jessica Zhang <[email protected]>
>>>>>> ---
>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 ++-
>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
>>>>>> 2 files changed, 4 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> 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 36ba3f58dcdf..0be7bf0bfc41 100644
>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>>>> @@ -103,7 +103,8 @@
>>>>>> (BIT(DPU_INTF_INPUT_CTRL) | \
>>>>>> BIT(DPU_INTF_TE) | \
>>>>>> BIT(DPU_INTF_STATUS_SUPPORTED) | \
>>>>>> - BIT(DPU_DATA_HCTL_EN))
>>>>>> + BIT(DPU_DATA_HCTL_EN) | \
>>>>>> + BIT(DPU_INTF_DATABUS_WIDEN))
>>>>>
>>>>> This doesn't work. DPU 5.0.0 is SM8150, which has DSI 6G 2.3. In the
>>>>> last patch for DSI you state and enable widebus for DSI 6G 2.5+ only,
>>>>> meaning DPU and DSI are now desynced, and the output is completely
>>>>> corrupted.
>>>
I looked at the internal docs and also this change. This change is
incorrect because this will try to enable widebus for DPU >= 5.0 and DSI
>= 2.5
That was not the intended right condition as thats not what the docs say.
We should enable for DPU >= 7.0 and DSI >= 2.5
Is there any combination where this compatibility is broken? That would
be the strange thing for me ( not DPU 5.0 and DSI 2.5 as that was incorrect)
Part of this confusion is because of catalog macro re-use again.
This series is a good candidate and infact I think we should only do
core_revision based check on DPU and DSI to avoid bringing the catalog
mess into this.
>>> Tested this on SM8350 which actually has DSI 2.5, and it is also
>>> corrupted with this series so something else on this series might be
>>> broken.
>>>
>
> Missed this response. That seems strange.
>
> This series was tested on SM8350 HDK with a command mode panel.
>
> We will fix the DPU-DSI handshake and post a v2 but your issue needs
> investigation in parallel.
>
> So another bug to track that would be great.
>
>>>>> Is the bound in dsi_host wrong, or do DPU and DSI need to communicate
>>>>> when widebus will be enabled, based on DPU && DSI supporting it?
>>>>
>>>> I'd prefer to follow the second approach, as we did for DP. DPU asks
>>>> the
>>>> actual video output driver if widebus is to be enabled.
>>>
>>
>> I was afraid of this. This series was made on an assumption that the
>> DPU version of widebus and DSI version of widebus would be compatible
>> but looks like already SM8150 is an outlier.
>>
>> Yes, I think we have to go with second approach.
>>
>> DPU queries DSI if it supports widebus and enables it.
>>
>> Thanks for your responses. We will post a v2.
>>
>>> Doesn't it seem very strange that DPU 5.x+ comes with a widebus feature,
>>> but the DSI does not until two revisions later? Or is this available on
>>> every interface, but only for a different (probably DP) encoder block?
>>>
>>
>> Yes its strange.
>>
I have clarified this above. Its not strange but appeared strange
because we were checking wrong conditions.
>>> - Marijn
On 14/06/2023 23:39, Abhinav Kumar wrote:
>
>
> On 6/14/2023 12:54 PM, Abhinav Kumar wrote:
>>
>>
>> On 6/14/2023 12:35 PM, Abhinav Kumar wrote:
>>>
>>>
>>> On 6/14/2023 5:23 AM, Marijn Suijten wrote:
>>>> On 2023-06-14 15:01:59, Dmitry Baryshkov wrote:
>>>>> On 14/06/2023 14:42, Marijn Suijten wrote:
>>>>>> On 2023-06-13 18:57:11, Jessica Zhang wrote:
>>>>>>> DPU 5.x+ supports a databus widen mode that allows more data to
>>>>>>> be sent
>>>>>>> per pclk. Enable this feature flag on all relevant chipsets.
>>>>>>>
>>>>>>> Signed-off-by: Jessica Zhang <[email protected]>
>>>>>>> ---
>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 ++-
>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
>>>>>>> 2 files changed, 4 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> 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 36ba3f58dcdf..0be7bf0bfc41 100644
>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>>>>> @@ -103,7 +103,8 @@
>>>>>>> (BIT(DPU_INTF_INPUT_CTRL) | \
>>>>>>> BIT(DPU_INTF_TE) | \
>>>>>>> BIT(DPU_INTF_STATUS_SUPPORTED) | \
>>>>>>> - BIT(DPU_DATA_HCTL_EN))
>>>>>>> + BIT(DPU_DATA_HCTL_EN) | \
>>>>>>> + BIT(DPU_INTF_DATABUS_WIDEN))
>>>>>>
>>>>>> This doesn't work. DPU 5.0.0 is SM8150, which has DSI 6G 2.3. In
>>>>>> the
>>>>>> last patch for DSI you state and enable widebus for DSI 6G 2.5+ only,
>>>>>> meaning DPU and DSI are now desynced, and the output is completely
>>>>>> corrupted.
>>>>
>
> I looked at the internal docs and also this change. This change is
> incorrect because this will try to enable widebus for DPU >= 5.0 and DSI
> >= 2.5
>
> That was not the intended right condition as thats not what the docs say.
>
> We should enable for DPU >= 7.0 and DSI >= 2.5
>
> Is there any combination where this compatibility is broken? That would
> be the strange thing for me ( not DPU 5.0 and DSI 2.5 as that was
> incorrect)
>
> Part of this confusion is because of catalog macro re-use again.
>
> This series is a good candidate and infact I think we should only do
> core_revision based check on DPU and DSI to avoid bringing the catalog
> mess into this.
I have just a single request here: can we please have the same approach
for both DSI and DP? I don't mind changing DP code if it makes it
better. If you don't have better reasons, I like the idea of DSI/DP
dictating whether wide bus should be used on the particular interface.
It allows us to handle possible errata or corner cases there. Another
option would be to make DPU tell DSI / DP whether the wide bus is
enabled or not, but I'd say, this is slightly worse solution.
>
>>>> Tested this on SM8350 which actually has DSI 2.5, and it is also
>>>> corrupted with this series so something else on this series might be
>>>> broken.
>>>>
>>
>> Missed this response. That seems strange.
>>
>> This series was tested on SM8350 HDK with a command mode panel.
>>
>> We will fix the DPU-DSI handshake and post a v2 but your issue needs
>> investigation in parallel.
>>
>> So another bug to track that would be great.
>>
>>>>>> Is the bound in dsi_host wrong, or do DPU and DSI need to communicate
>>>>>> when widebus will be enabled, based on DPU && DSI supporting it?
>>>>>
>>>>> I'd prefer to follow the second approach, as we did for DP. DPU
>>>>> asks the
>>>>> actual video output driver if widebus is to be enabled.
>>>>
>>>
>>> I was afraid of this. This series was made on an assumption that the
>>> DPU version of widebus and DSI version of widebus would be compatible
>>> but looks like already SM8150 is an outlier.
>>>
>>> Yes, I think we have to go with second approach.
>>>
>>> DPU queries DSI if it supports widebus and enables it.
>>>
>>> Thanks for your responses. We will post a v2.
>>>
>>>> Doesn't it seem very strange that DPU 5.x+ comes with a widebus
>>>> feature,
>>>> but the DSI does not until two revisions later? Or is this
>>>> available on
>>>> every interface, but only for a different (probably DP) encoder block?
>>>>
>>>
>>> Yes its strange.
>>>
>
> I have clarified this above. Its not strange but appeared strange
> because we were checking wrong conditions.
>
>
>>>> - Marijn
--
With best wishes
Dmitry
On 2023-06-14 13:39:57, Abhinav Kumar wrote:
> On 6/14/2023 12:54 PM, Abhinav Kumar wrote:
> > On 6/14/2023 12:35 PM, Abhinav Kumar wrote:
> >> On 6/14/2023 5:23 AM, Marijn Suijten wrote:
> >>> On 2023-06-14 15:01:59, Dmitry Baryshkov wrote:
> >>>> On 14/06/2023 14:42, Marijn Suijten wrote:
> >>>>> On 2023-06-13 18:57:11, Jessica Zhang wrote:
> >>>>>> DPU 5.x+ supports a databus widen mode that allows more data to be
> >>>>>> sent
> >>>>>> per pclk. Enable this feature flag on all relevant chipsets.
> >>>>>>
> >>>>>> Signed-off-by: Jessica Zhang <[email protected]>
> >>>>>> ---
> >>>>>> ?? drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 ++-
> >>>>>> ?? drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
> >>>>>> ?? 2 files changed, 4 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> 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 36ba3f58dcdf..0be7bf0bfc41 100644
> >>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >>>>>> @@ -103,7 +103,8 @@
> >>>>>> ?????? (BIT(DPU_INTF_INPUT_CTRL) | \
> >>>>>> ??????? BIT(DPU_INTF_TE) | \
> >>>>>> ??????? BIT(DPU_INTF_STATUS_SUPPORTED) | \
> >>>>>> -???? BIT(DPU_DATA_HCTL_EN))
> >>>>>> +???? BIT(DPU_DATA_HCTL_EN) | \
> >>>>>> +???? BIT(DPU_INTF_DATABUS_WIDEN))
> >>>>>
> >>>>> This doesn't work.? DPU 5.0.0 is SM8150, which has DSI 6G 2.3.? In the
> >>>>> last patch for DSI you state and enable widebus for DSI 6G 2.5+ only,
> >>>>> meaning DPU and DSI are now desynced, and the output is completely
> >>>>> corrupted.
> >>>
>
> I looked at the internal docs and also this change. This change is
> incorrect because this will try to enable widebus for DPU >= 5.0 and DSI
> >= 2.5
>
> That was not the intended right condition as thats not what the docs say.
>
> We should enable for DPU >= 7.0 and DSI >= 2.5
That makes more sense, DSI 2.5 is SM8350 which has DPU 7.0.
> Is there any combination where this compatibility is broken? That would
> be the strange thing for me ( not DPU 5.0 and DSI 2.5 as that was incorrect)
No clue if there are any interim SoCs...
> Part of this confusion is because of catalog macro re-use again.
Somewhat agreed. SC7180 is a DPU 6.2 SoC, and for this mask to be used
across DPU 5.x and above it should have been renamed to SM8150 and as
suggested by Dmitry, have DPU_5_x_` as suffix.
As I've asked many times before, we should inline these masks (not just
the macros) (disclaimer: haven't reviewed if Dmitry's series actually
does so!).
> This series is a good candidate and infact I think we should only do
> core_revision based check on DPU and DSI to avoid bringing the catalog
> mess into this.
>
> >>> Tested this on SM8350 which actually has DSI 2.5, and it is also
> >>> corrupted with this series so something else on this series might be
> >>> broken.
> >>>
> >
> > Missed this response. That seems strange.
No worries. But don't forget to look at the comments on patch 2/3
either. Some of it is a continuation of pclk scaling factor for DSC
which discussion seems to have ceased on.
> > This series was tested on SM8350 HDK with a command mode panel.
> >
> > We will fix the DPU-DSI handshake and post a v2 but your issue needs
> > investigation in parallel.
> >
> > So another bug to track that would be great.
Will see if I can set that up for you!
> >>>>> Is the bound in dsi_host wrong, or do DPU and DSI need to communicate
> >>>>> when widebus will be enabled, based on DPU && DSI supporting it?
> >>>>
> >>>> I'd prefer to follow the second approach, as we did for DP. DPU asks
> >>>> the
> >>>> actual video output driver if widebus is to be enabled.
> >>>
> >>
> >> I was afraid of this. This series was made on an assumption that the
> >> DPU version of widebus and DSI version of widebus would be compatible
> >> but looks like already SM8150 is an outlier.
Fwiw SM8250 would have been an outlier as well :)
> >> Yes, I think we have to go with second approach.
> >>
> >> DPU queries DSI if it supports widebus and enables it.
> >>
> >> Thanks for your responses. We will post a v2.
No hurry, btw. As alluded to above, let's also look at the comments on
patch 2/3 and discuss how this affects pclk.
> >>> Doesn't it seem very strange that DPU 5.x+ comes with a widebus feature,
> >>> but the DSI does not until two revisions later?? Or is this available on
> >>> every interface, but only for a different (probably DP) encoder block?
> >>>
> >>
> >> Yes its strange.
> >>
>
> I have clarified this above. Its not strange but appeared strange
> because we were checking wrong conditions.
Hehe :)
- Marijn
On 2023-06-14 14:23:38, Marijn Suijten wrote:
<snip>
> Tested this on SM8350 which actually has DSI 2.5, and it is also
> corrupted with this series so something else on this series might be
> broken.
Never mind, this was a bad conflict-resolve. Jessica's original
BURST_MODE patch was RMW'ing MDP_CTRL2, but the upstreamed patch was
only writing, and the way I conflict-resolved that caused the write of
BURST_MODE to overwrite the RMW DATABUS_WIDEN.
If both are moved to dsi_ctrl_config(), we could do a read, add both
flags in conditionally, and write.
- Marijn
On 14/06/2023 04:57, Jessica Zhang wrote:
> DPU 5.x+ and DSI 6G 2.5.x+ support a databus-widen mode that allows for
> more compressed data to be transferred per pclk.
>
> This series adds support for enabling this feature for both DPU and DSI
> by doing the following:
>
> 1. Add a DPU_INTF_DATABUS_WIDEN feature flag
> 2. Add a DPU INTF op to set the DATABUS_WIDEN register
> 3. Set the DATABUS_WIDEN register and do the proper hdisplay
> calculations in DSI when applicable
As I was writing the documentation patch, another thought stroke me wrt
this patchset. Could you please add a check to DSI's mode_valid
disallowing all modes if DSI_BPP > 8 & !widebus. Technically this check
does not filter modes per se, but in my opinion ending up with empty
modes list would be a better user experience compared to having a list
of modes, from which none can be selected.
And if we ever get dynamic mode/dsc_config setup, this would allow us to
filter unsupported modes via their DSC confiuration.
Abhinav, I remember, that you told me that adding drm_dsc_config to drm
modes was forbidden already. However this looks like a valid reason to
maybe restart the discussion: we want to filter modes basing on the
corresponding DSC data rather than allowing the mode in mode_valid() and
then failing it in the atomic_check().
>
> Note: This series will only enable the databus-widen mode for command
> mode as we are currently unable to validate it on video mode.
>
> Depends on: "Add DSC v1.2 Support for DSI" [1]
>
> [1] https://patchwork.freedesktop.org/series/117219/
>
> Signed-off-by: Jessica Zhang <[email protected]>
Nit: there is no need to sign-off the cover letters.
> ---
> Jessica Zhang (3):
> drm/msm/dpu: Add DPU_INTF_DATABUS_WIDEN feature flag for DPU >= 5.0
> drm/msm/dpu: Set DATABUS_WIDEN on command mode encoders
> drm/msm/dsi: Enable DATABUS_WIDEN for DSI command mode
>
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 3 +++
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 ++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 12 ++++++++++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 3 +++
> drivers/gpu/drm/msm/dsi/dsi.xml.h | 1 +
> drivers/gpu/drm/msm/dsi/dsi_host.c | 19 ++++++++++++++++++-
> 7 files changed, 41 insertions(+), 2 deletions(-)
> ---
> base-commit: 1981c2c0c05f5d7fe4d4552d4f352cb46840e51e
> change-id: 20230525-add-widebus-support-f785546ee751
>
> Best regards,
--
With best wishes
Dmitry
On 6/14/2023 3:49 PM, Marijn Suijten wrote:
> On 2023-06-14 14:23:38, Marijn Suijten wrote:
> <snip>
>> Tested this on SM8350 which actually has DSI 2.5, and it is also
>> corrupted with this series so something else on this series might be
>> broken.
>
> Never mind, this was a bad conflict-resolve. Jessica's original
> BURST_MODE patch was RMW'ing MDP_CTRL2, but the upstreamed patch was
> only writing, and the way I conflict-resolved that caused the write of
> BURST_MODE to overwrite the RMW DATABUS_WIDEN.
>
> If both are moved to dsi_ctrl_config(), we could do a read, add both
> flags in conditionally, and write.
>
So just to confirm, there is no issue on your 8350 setup with widebus
enabled right?
> - Marijn
On 6/14/2023 1:43 PM, Dmitry Baryshkov wrote:
> On 14/06/2023 23:39, Abhinav Kumar wrote:
>>
>>
>> On 6/14/2023 12:54 PM, Abhinav Kumar wrote:
>>>
>>>
>>> On 6/14/2023 12:35 PM, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 6/14/2023 5:23 AM, Marijn Suijten wrote:
>>>>> On 2023-06-14 15:01:59, Dmitry Baryshkov wrote:
>>>>>> On 14/06/2023 14:42, Marijn Suijten wrote:
>>>>>>> On 2023-06-13 18:57:11, Jessica Zhang wrote:
>>>>>>>> DPU 5.x+ supports a databus widen mode that allows more data to
>>>>>>>> be sent
>>>>>>>> per pclk. Enable this feature flag on all relevant chipsets.
>>>>>>>>
>>>>>>>> Signed-off-by: Jessica Zhang <[email protected]>
>>>>>>>> ---
>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 ++-
>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
>>>>>>>> 2 files changed, 4 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> 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 36ba3f58dcdf..0be7bf0bfc41 100644
>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>>>>>> @@ -103,7 +103,8 @@
>>>>>>>> (BIT(DPU_INTF_INPUT_CTRL) | \
>>>>>>>> BIT(DPU_INTF_TE) | \
>>>>>>>> BIT(DPU_INTF_STATUS_SUPPORTED) | \
>>>>>>>> - BIT(DPU_DATA_HCTL_EN))
>>>>>>>> + BIT(DPU_DATA_HCTL_EN) | \
>>>>>>>> + BIT(DPU_INTF_DATABUS_WIDEN))
>>>>>>>
>>>>>>> This doesn't work. DPU 5.0.0 is SM8150, which has DSI 6G 2.3.
>>>>>>> In the
>>>>>>> last patch for DSI you state and enable widebus for DSI 6G 2.5+
>>>>>>> only,
>>>>>>> meaning DPU and DSI are now desynced, and the output is completely
>>>>>>> corrupted.
>>>>>
>>
>> I looked at the internal docs and also this change. This change is
>> incorrect because this will try to enable widebus for DPU >= 5.0 and
>> DSI >= 2.5
>>
>> That was not the intended right condition as thats not what the docs say.
>>
>> We should enable for DPU >= 7.0 and DSI >= 2.5
>>
>> Is there any combination where this compatibility is broken? That
>> would be the strange thing for me ( not DPU 5.0 and DSI 2.5 as that
>> was incorrect)
>>
>> Part of this confusion is because of catalog macro re-use again.
>>
>> This series is a good candidate and infact I think we should only do
>> core_revision based check on DPU and DSI to avoid bringing the catalog
>> mess into this.
>
> I have just a single request here: can we please have the same approach
> for both DSI and DP? I don't mind changing DP code if it makes it
> better. If you don't have better reasons, I like the idea of DSI/DP
> dictating whether wide bus should be used on the particular interface.
> It allows us to handle possible errata or corner cases there. Another
> option would be to make DPU tell DSI / DP whether the wide bus is
> enabled or not, but I'd say, this is slightly worse solution.
>
Today, DP's widebus does not check if DPU supports that or not.
DPU encoder queries the DP whether widebus is available and enables it.
We can also do the same thing for DSI.
So for intf_type of DSI, DPU encoder will query DSI if it supports widebus.
Then DSI will do its version checks and for DSC it will say yes.
This way, we will never check for the DPU's core revision for DSI and
purely rely of DP/DSI's hw revisions.
Thats fine with me because that way we again just rely on the hw
revision to enable the feature.
But as a result I am still going to drop this patch which adds widebus
to the catalog as a dpu cap which I always wanted to do anyway as we
will just rely on the DSI and DP hw revisions.
>>
>>>>> Tested this on SM8350 which actually has DSI 2.5, and it is also
>>>>> corrupted with this series so something else on this series might be
>>>>> broken.
>>>>>
>>>
>>> Missed this response. That seems strange.
>>>
>>> This series was tested on SM8350 HDK with a command mode panel.
>>>
>>> We will fix the DPU-DSI handshake and post a v2 but your issue needs
>>> investigation in parallel.
>>>
>>> So another bug to track that would be great.
>>>
>>>>>>> Is the bound in dsi_host wrong, or do DPU and DSI need to
>>>>>>> communicate
>>>>>>> when widebus will be enabled, based on DPU && DSI supporting it?
>>>>>>
>>>>>> I'd prefer to follow the second approach, as we did for DP. DPU
>>>>>> asks the
>>>>>> actual video output driver if widebus is to be enabled.
>>>>>
>>>>
>>>> I was afraid of this. This series was made on an assumption that the
>>>> DPU version of widebus and DSI version of widebus would be
>>>> compatible but looks like already SM8150 is an outlier.
>>>>
>>>> Yes, I think we have to go with second approach.
>>>>
>>>> DPU queries DSI if it supports widebus and enables it.
>>>>
>>>> Thanks for your responses. We will post a v2.
>>>>
>>>>> Doesn't it seem very strange that DPU 5.x+ comes with a widebus
>>>>> feature,
>>>>> but the DSI does not until two revisions later? Or is this
>>>>> available on
>>>>> every interface, but only for a different (probably DP) encoder block?
>>>>>
>>>>
>>>> Yes its strange.
>>>>
>>
>> I have clarified this above. Its not strange but appeared strange
>> because we were checking wrong conditions.
>>
>>
>>>>> - Marijn
>
On 6/14/2023 2:41 PM, Marijn Suijten wrote:
> On 2023-06-14 13:39:57, Abhinav Kumar wrote:
>> On 6/14/2023 12:54 PM, Abhinav Kumar wrote:
>>> On 6/14/2023 12:35 PM, Abhinav Kumar wrote:
>>>> On 6/14/2023 5:23 AM, Marijn Suijten wrote:
>>>>> On 2023-06-14 15:01:59, Dmitry Baryshkov wrote:
>>>>>> On 14/06/2023 14:42, Marijn Suijten wrote:
>>>>>>> On 2023-06-13 18:57:11, Jessica Zhang wrote:
>>>>>>>> DPU 5.x+ supports a databus widen mode that allows more data to be
>>>>>>>> sent
>>>>>>>> per pclk. Enable this feature flag on all relevant chipsets.
>>>>>>>>
>>>>>>>> Signed-off-by: Jessica Zhang <[email protected]>
>>>>>>>> ---
>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 ++-
>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
>>>>>>>> 2 files changed, 4 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> 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 36ba3f58dcdf..0be7bf0bfc41 100644
>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>>>>>> @@ -103,7 +103,8 @@
>>>>>>>> (BIT(DPU_INTF_INPUT_CTRL) | \
>>>>>>>> BIT(DPU_INTF_TE) | \
>>>>>>>> BIT(DPU_INTF_STATUS_SUPPORTED) | \
>>>>>>>> - BIT(DPU_DATA_HCTL_EN))
>>>>>>>> + BIT(DPU_DATA_HCTL_EN) | \
>>>>>>>> + BIT(DPU_INTF_DATABUS_WIDEN))
>>>>>>>
>>>>>>> This doesn't work. DPU 5.0.0 is SM8150, which has DSI 6G 2.3. In the
>>>>>>> last patch for DSI you state and enable widebus for DSI 6G 2.5+ only,
>>>>>>> meaning DPU and DSI are now desynced, and the output is completely
>>>>>>> corrupted.
>>>>>
>>
>> I looked at the internal docs and also this change. This change is
>> incorrect because this will try to enable widebus for DPU >= 5.0 and DSI
>> >= 2.5
>>
>> That was not the intended right condition as thats not what the docs say.
>>
>> We should enable for DPU >= 7.0 and DSI >= 2.5
>
> That makes more sense, DSI 2.5 is SM8350 which has DPU 7.0.
>
>> Is there any combination where this compatibility is broken? That would
>> be the strange thing for me ( not DPU 5.0 and DSI 2.5 as that was incorrect)
>
> No clue if there are any interim SoCs...
>
>> Part of this confusion is because of catalog macro re-use again.
>
> Somewhat agreed. SC7180 is a DPU 6.2 SoC, and for this mask to be used
> across DPU 5.x and above it should have been renamed to SM8150 and as
> suggested by Dmitry, have DPU_5_x_` as suffix.
>
> As I've asked many times before, we should inline these masks (not just
> the macros) (disclaimer: haven't reviewed if Dmitry's series actually
> does so!).
>
Yes it does inline it. I am halfway through that rework got stuck in one
of the patches.
>> This series is a good candidate and infact I think we should only do
>> core_revision based check on DPU and DSI to avoid bringing the catalog
>> mess into this.
>>
>>>>> Tested this on SM8350 which actually has DSI 2.5, and it is also
>>>>> corrupted with this series so something else on this series might be
>>>>> broken.
>>>>>
>>>
>>> Missed this response. That seems strange.
>
> No worries. But don't forget to look at the comments on patch 2/3
> either. Some of it is a continuation of pclk scaling factor for DSC
> which discussion seems to have ceased on.
>
>>> This series was tested on SM8350 HDK with a command mode panel.
>>>
>>> We will fix the DPU-DSI handshake and post a v2 but your issue needs
>>> investigation in parallel.
>>>
>>> So another bug to track that would be great.
>
> Will see if I can set that up for you!
>
Now, it seems this is resolved so bug is not needed?
>>>>>>> Is the bound in dsi_host wrong, or do DPU and DSI need to communicate
>>>>>>> when widebus will be enabled, based on DPU && DSI supporting it?
>>>>>>
>>>>>> I'd prefer to follow the second approach, as we did for DP. DPU asks
>>>>>> the
>>>>>> actual video output driver if widebus is to be enabled.
>>>>>
>>>>
>>>> I was afraid of this. This series was made on an assumption that the
>>>> DPU version of widebus and DSI version of widebus would be compatible
>>>> but looks like already SM8150 is an outlier.
>
> Fwiw SM8250 would have been an outlier as well :)
>
>>>> Yes, I think we have to go with second approach.
>>>>
>>>> DPU queries DSI if it supports widebus and enables it.
>>>>
>>>> Thanks for your responses. We will post a v2.
>
> No hurry, btw. As alluded to above, let's also look at the comments on
> patch 2/3 and discuss how this affects pclk.
>
>>>>> Doesn't it seem very strange that DPU 5.x+ comes with a widebus feature,
>>>>> but the DSI does not until two revisions later? Or is this available on
>>>>> every interface, but only for a different (probably DP) encoder block?
>>>>>
>>>>
>>>> Yes its strange.
>>>>
>>
>> I have clarified this above. Its not strange but appeared strange
>> because we were checking wrong conditions.
>
> Hehe :)
>
> - Marijn
On 6/14/2023 12:49 AM, Dmitry Baryshkov wrote:
> On 14/06/2023 04:57, Jessica Zhang wrote:
>> DSI 6G v2.5.x+ supports a data-bus widen mode that allows DSI to send
>> 48 bits of compressed data per pclk instead of 24.
>>
>> For all chipsets that support this mode, enable it whenever DSC is
>> enabled as recommend by the hardware programming guide.
>>
>> Only enable this for command mode as we are currently unable to validate
>> it for video mode.
>>
>> Signed-off-by: Jessica Zhang <[email protected]>
>> ---
>>
>> Note: The dsi.xml.h changes were generated using the headergen2 script in
>> envytools [1], but the changes to the copyright and rules-ng-ng source
>> file
>> paths were dropped.
>>
>> [1] https://github.com/freedreno/envytools/
>>
>> drivers/gpu/drm/msm/dsi/dsi.xml.h | 1 +
>> drivers/gpu/drm/msm/dsi/dsi_host.c | 19 ++++++++++++++++++-
>> 2 files changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h
>> b/drivers/gpu/drm/msm/dsi/dsi.xml.h
>> index a4a154601114..2a7d980e12c3 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
>> +++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
>> @@ -664,6 +664,7 @@ static inline uint32_t
>> DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP(enum dsi_rgb_swap v
>> return ((val) << DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__SHIFT) &
>> DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__MASK;
>> }
>> #define DSI_CMD_MODE_MDP_CTRL2_BURST_MODE 0x00010000
>> +#define DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN 0x00100000
>>
>> #define REG_DSI_CMD_MODE_MDP_STREAM2_CTRL 0x000001b8
>> #define DSI_CMD_MODE_MDP_STREAM2_CTRL_DATA_TYPE__MASK 0x0000003f
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index 5d7b4409e4e9..1da5238e7105 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -927,6 +927,9 @@ static void dsi_timing_setup(struct msm_dsi_host
>> *msm_host, bool is_bonded_dsi)
>> u32 hdisplay = mode->hdisplay;
>> u32 wc;
>> int ret;
>> + bool widebus_supported = msm_host->cfg_hnd->major ==
>> MSM_DSI_VER_MAJOR_6G &&
>> + msm_host->cfg_hnd->minor >= MSM_DSI_6G_VER_MINOR_V2_5_0;
>> +
>>
>> DBG("");
>>
>> @@ -973,8 +976,15 @@ static void dsi_timing_setup(struct msm_dsi_host
>> *msm_host, bool is_bonded_dsi)
>> *
>> * hdisplay will be divided by 3 here to account for the fact
>> * that DPU sends 3 bytes per pclk cycle to DSI.
>> + *
>> + * If widebus is supported, set DATABUS_WIDEN register and
>> divide hdisplay by 6
>> + * instead of 3
>
> This is useless, it is already obvious from the code below. Instead
> there should be something like "wide bus extends that to 6 bytes per
> pclk cycle"
>
>> */
>> - hdisplay =
>> DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
>> + if (!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) &&
>> widebus_supported)
>> + hdisplay =
>> DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 6);
>> + else
>> + hdisplay =
>> DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
>> +
>> h_total += hdisplay;
>> ha_end = ha_start + hdisplay;
>> }
>> @@ -1027,6 +1037,13 @@ static void dsi_timing_setup(struct
>> msm_dsi_host *msm_host, bool is_bonded_dsi)
>> dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_TOTAL,
>> DSI_CMD_MDP_STREAM0_TOTAL_H_TOTAL(hdisplay) |
>> DSI_CMD_MDP_STREAM0_TOTAL_V_TOTAL(mode->vdisplay));
>> +
>> + if (msm_host->dsc && widebus_supported) {
>> + u32 mdp_ctrl2 = dsi_read(msm_host,
>> REG_DSI_CMD_MODE_MDP_CTRL2);
>> +
>> + mdp_ctrl2 |= DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN;
>> + dsi_write(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2, mdp_ctrl2);
>
> Is widebus applicable only to the CMD mode, or video mode can employ it
> too?
Video mode can employ it too but like Jessica said in the commit text,
we dont have a setup to validate this for DSI video mode so it was
restricted to cmd mode.
We can leave a note here too.
>
>> + }
>> }
>> }
>>
>>
>> --
>> 2.40.1
>>
>
On 6/14/2023 12:56 AM, Dmitry Baryshkov wrote:
> On 14/06/2023 04:57, Jessica Zhang wrote:
>> Add a DPU INTF op to set the DATABUS_WIDEN register to enable the
>> databus-widen mode datapath.
>>
>> Signed-off-by: Jessica Zhang <[email protected]>
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 3 +++
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 12 ++++++++++++
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 3 +++
>> 3 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> index b856c6286c85..124ba96bebda 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> @@ -70,6 +70,9 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>> if (intf_cfg.dsc != 0 && phys_enc->hw_intf->ops.enable_compression)
>> phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
>> +
>> + if (phys_enc->hw_intf->ops.enable_widebus)
>> + phys_enc->hw_intf->ops.enable_widebus(phys_enc->hw_intf);
>
> No. Please provide a single function which takes necessary
> configuration, including compression and wide_bus_enable.
>
There are two ways to look at this. Your point is coming from the
perspective that its programming the same register but just a different
bit. But that will also make it a bit confusing.
So lets say the function is called intf_cfg2_xxx(..., bool widebus, bool
data_compress)
Now for the caller who wants to just enable widebus this will be
intf_cfg2_xxx(....., true, false)
if we want to do both
intf_cfg2_xxx(...., true, true)
the last one where we want to do just data_compress(highly unlikely and
not recommended)
intf_cfg2_xxx(...., false, true)
Now someone looking at the code will have to go and find out what each
bool is.
Whereas with separate ops, its kind of implicit.
For that reason, I dont think this patch is bad at all.
On 2023-06-16 14:06:47, Abhinav Kumar wrote:
> On 6/14/2023 3:49 PM, Marijn Suijten wrote:
> > On 2023-06-14 14:23:38, Marijn Suijten wrote:
> > <snip>
> >> Tested this on SM8350 which actually has DSI 2.5, and it is also
> >> corrupted with this series so something else on this series might be
> >> broken.
> >
> > Never mind, this was a bad conflict-resolve. Jessica's original
> > BURST_MODE patch was RMW'ing MDP_CTRL2, but the upstreamed patch was
> > only writing, and the way I conflict-resolved that caused the write of
> > BURST_MODE to overwrite the RMW DATABUS_WIDEN.
> >
> > If both are moved to dsi_ctrl_config(), we could do a read, add both
> > flags in conditionally, and write.
> >
>
> So just to confirm, there is no issue on your 8350 setup with widebus
> enabled right?
After properly conflict-resolving both series so that they either both
RMW, or combine the two bit-sets (the same you just discussed _not_
doing on the DPU side for DATABUS_WIDEN|COMPRESSION...) before a single
write, it works fine!
- Marijn
On 2023-06-16 14:13:22, Abhinav Kumar wrote:
<snip>
> > As I've asked many times before, we should inline these masks (not just
> > the macros) (disclaimer: haven't reviewed if Dmitry's series actually
> > does so!).
> >
>
> Yes it does inline it. I am halfway through that rework got stuck in one
> of the patches.
Neat, I'm still going through it but there are some conflicts with other
reworks that make it harder to review and test in parallel.
> >>>>> Tested this on SM8350 which actually has DSI 2.5, and it is also
> >>>>> corrupted with this series so something else on this series might be
> >>>>> broken.
> >>>>>
> >>>
> >>> Missed this response. That seems strange.
> >
> > No worries. But don't forget to look at the comments on patch 2/3
> > either. Some of it is a continuation of pclk scaling factor for DSC
> > which discussion seems to have ceased on.
> >
> >>> This series was tested on SM8350 HDK with a command mode panel.
> >>>
> >>> We will fix the DPU-DSI handshake and post a v2 but your issue needs
> >>> investigation in parallel.
> >>>
> >>> So another bug to track that would be great.
> >
> > Will see if I can set that up for you!
> >
>
> Now, it seems this is resolved so bug is not needed?
Indeed, as mentioned in another message.? Looking forward to v2.
- Marijn
On 2023-06-16 14:18:39, Abhinav Kumar wrote:
>
>
> On 6/14/2023 12:56 AM, Dmitry Baryshkov wrote:
> > On 14/06/2023 04:57, Jessica Zhang wrote:
> >> Add a DPU INTF op to set the DATABUS_WIDEN register to enable the
> >> databus-widen mode datapath.
> >>
> >> Signed-off-by: Jessica Zhang <[email protected]>
> >> ---
> >> ? drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c |? 3 +++
> >> ? drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c????????? | 12 ++++++++++++
> >> ? drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h????????? |? 3 +++
> >> ? 3 files changed, 18 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> >> index b856c6286c85..124ba96bebda 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> >> @@ -70,6 +70,9 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
> >> ????? if (intf_cfg.dsc != 0 && phys_enc->hw_intf->ops.enable_compression)
> >> ????????? phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
> >> +
> >> +??? if (phys_enc->hw_intf->ops.enable_widebus)
> >> +??????? phys_enc->hw_intf->ops.enable_widebus(phys_enc->hw_intf);
> >
> > No. Please provide a single function which takes necessary
> > configuration, including compression and wide_bus_enable.
> >
>
> There are two ways to look at this. Your point is coming from the
> perspective that its programming the same register but just a different
> bit. But that will also make it a bit confusing.
>
> So lets say the function is called intf_cfg2_xxx(..., bool widebus, bool
> data_compress)
>
> Now for the caller who wants to just enable widebus this will be
>
> intf_cfg2_xxx(....., true, false)
>
> if we want to do both
>
> intf_cfg2_xxx(...., true, true)
>
> the last one where we want to do just data_compress(highly unlikely and
> not recommended)
>
> intf_cfg2_xxx(...., false, true)
>
> Now someone looking at the code will have to go and find out what each
> bool is.
>
> Whereas with separate ops, its kind of implicit.
That's why you never pass bools as function argument (edge-case if it is
the only argument, and its meaning becomes clear from the function
name). Use enumerations anywhere else.
- Marijn
>
> For that reason, I dont think this patch is bad at all.
On 17/06/2023 00:54, Marijn Suijten wrote:
> On 2023-06-16 14:18:39, Abhinav Kumar wrote:
>>
>>
>> On 6/14/2023 12:56 AM, Dmitry Baryshkov wrote:
>>> On 14/06/2023 04:57, Jessica Zhang wrote:
>>>> Add a DPU INTF op to set the DATABUS_WIDEN register to enable the
>>>> databus-widen mode datapath.
>>>>
>>>> Signed-off-by: Jessica Zhang <[email protected]>
>>>> ---
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 3 +++
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 12 ++++++++++++
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 3 +++
>>>> 3 files changed, 18 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> index b856c6286c85..124ba96bebda 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>> @@ -70,6 +70,9 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>>> if (intf_cfg.dsc != 0 && phys_enc->hw_intf->ops.enable_compression)
>>>> phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
>>>> +
>>>> + if (phys_enc->hw_intf->ops.enable_widebus)
>>>> + phys_enc->hw_intf->ops.enable_widebus(phys_enc->hw_intf);
>>>
>>> No. Please provide a single function which takes necessary
>>> configuration, including compression and wide_bus_enable.
>>>
>>
>> There are two ways to look at this. Your point is coming from the
>> perspective that its programming the same register but just a different
>> bit. But that will also make it a bit confusing.
My point is to have a high-level function that configures the INTF for
the CMD mode. This way it can take a structure with necessary
configuration bits.
>>
>> So lets say the function is called intf_cfg2_xxx(..., bool widebus, bool
>> data_compress)
>>
>> Now for the caller who wants to just enable widebus this will be
>>
>> intf_cfg2_xxx(....., true, false)
>>
>> if we want to do both
>>
>> intf_cfg2_xxx(...., true, true)
>>
>> the last one where we want to do just data_compress(highly unlikely and
>> not recommended)
>>
>> intf_cfg2_xxx(...., false, true)
>>
>> Now someone looking at the code will have to go and find out what each
>> bool is.
>>
>> Whereas with separate ops, its kind of implicit.
>
> That's why you never pass bools as function argument (edge-case if it is
> the only argument, and its meaning becomes clear from the function
> name). Use enumerations anywhere else.
>
> - Marijn
>
>>
>> For that reason, I dont think this patch is bad at all.
--
With best wishes
Dmitry
On 17/06/2023 00:10, Abhinav Kumar wrote:
>
>
> On 6/14/2023 1:43 PM, Dmitry Baryshkov wrote:
>> On 14/06/2023 23:39, Abhinav Kumar wrote:
>>>
>>>
>>> On 6/14/2023 12:54 PM, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 6/14/2023 12:35 PM, Abhinav Kumar wrote:
>>>>>
>>>>>
>>>>> On 6/14/2023 5:23 AM, Marijn Suijten wrote:
>>>>>> On 2023-06-14 15:01:59, Dmitry Baryshkov wrote:
>>>>>>> On 14/06/2023 14:42, Marijn Suijten wrote:
>>>>>>>> On 2023-06-13 18:57:11, Jessica Zhang wrote:
>>>>>>>>> DPU 5.x+ supports a databus widen mode that allows more data to
>>>>>>>>> be sent
>>>>>>>>> per pclk. Enable this feature flag on all relevant chipsets.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Jessica Zhang <[email protected]>
>>>>>>>>> ---
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 ++-
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
>>>>>>>>> 2 files changed, 4 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> 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 36ba3f58dcdf..0be7bf0bfc41 100644
>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>>>>>>> @@ -103,7 +103,8 @@
>>>>>>>>> (BIT(DPU_INTF_INPUT_CTRL) | \
>>>>>>>>> BIT(DPU_INTF_TE) | \
>>>>>>>>> BIT(DPU_INTF_STATUS_SUPPORTED) | \
>>>>>>>>> - BIT(DPU_DATA_HCTL_EN))
>>>>>>>>> + BIT(DPU_DATA_HCTL_EN) | \
>>>>>>>>> + BIT(DPU_INTF_DATABUS_WIDEN))
>>>>>>>>
>>>>>>>> This doesn't work. DPU 5.0.0 is SM8150, which has DSI 6G 2.3.
>>>>>>>> In the
>>>>>>>> last patch for DSI you state and enable widebus for DSI 6G 2.5+
>>>>>>>> only,
>>>>>>>> meaning DPU and DSI are now desynced, and the output is completely
>>>>>>>> corrupted.
>>>>>>
>>>
>>> I looked at the internal docs and also this change. This change is
>>> incorrect because this will try to enable widebus for DPU >= 5.0 and
>>> DSI >= 2.5
>>>
>>> That was not the intended right condition as thats not what the docs
>>> say.
>>>
>>> We should enable for DPU >= 7.0 and DSI >= 2.5
>>>
>>> Is there any combination where this compatibility is broken? That
>>> would be the strange thing for me ( not DPU 5.0 and DSI 2.5 as that
>>> was incorrect)
>>>
>>> Part of this confusion is because of catalog macro re-use again.
>>>
>>> This series is a good candidate and infact I think we should only do
>>> core_revision based check on DPU and DSI to avoid bringing the
>>> catalog mess into this.
>>
>> I have just a single request here: can we please have the same
>> approach for both DSI and DP? I don't mind changing DP code if it
>> makes it better. If you don't have better reasons, I like the idea of
>> DSI/DP dictating whether wide bus should be used on the particular
>> interface. It allows us to handle possible errata or corner cases
>> there. Another option would be to make DPU tell DSI / DP whether the
>> wide bus is enabled or not, but I'd say, this is slightly worse solution.
>>
>
> Today, DP's widebus does not check if DPU supports that or not.
>
> DPU encoder queries the DP whether widebus is available and enables it.
>
> We can also do the same thing for DSI.
>
> So for intf_type of DSI, DPU encoder will query DSI if it supports widebus.
Not if it supports wide bus. But the check is whether enabling wide bus
is requested by the output driver (DSI/DP).
>
> Then DSI will do its version checks and for DSC it will say yes.
>
> This way, we will never check for the DPU's core revision for DSI and
> purely rely of DP/DSI's hw revisions.
>
> Thats fine with me because that way we again just rely on the hw
> revision to enable the feature.
>
> But as a result I am still going to drop this patch which adds widebus
> to the catalog as a dpu cap which I always wanted to do anyway as we
> will just rely on the DSI and DP hw revisions.
Yep.
>
>>>
>>>>>> Tested this on SM8350 which actually has DSI 2.5, and it is also
>>>>>> corrupted with this series so something else on this series might be
>>>>>> broken.
>>>>>>
>>>>
>>>> Missed this response. That seems strange.
>>>>
>>>> This series was tested on SM8350 HDK with a command mode panel.
>>>>
>>>> We will fix the DPU-DSI handshake and post a v2 but your issue needs
>>>> investigation in parallel.
>>>>
>>>> So another bug to track that would be great.
>>>>
>>>>>>>> Is the bound in dsi_host wrong, or do DPU and DSI need to
>>>>>>>> communicate
>>>>>>>> when widebus will be enabled, based on DPU && DSI supporting it?
>>>>>>>
>>>>>>> I'd prefer to follow the second approach, as we did for DP. DPU
>>>>>>> asks the
>>>>>>> actual video output driver if widebus is to be enabled.
>>>>>>
>>>>>
>>>>> I was afraid of this. This series was made on an assumption that
>>>>> the DPU version of widebus and DSI version of widebus would be
>>>>> compatible but looks like already SM8150 is an outlier.
>>>>>
>>>>> Yes, I think we have to go with second approach.
>>>>>
>>>>> DPU queries DSI if it supports widebus and enables it.
>>>>>
>>>>> Thanks for your responses. We will post a v2.
>>>>>
>>>>>> Doesn't it seem very strange that DPU 5.x+ comes with a widebus
>>>>>> feature,
>>>>>> but the DSI does not until two revisions later? Or is this
>>>>>> available on
>>>>>> every interface, but only for a different (probably DP) encoder
>>>>>> block?
>>>>>>
>>>>>
>>>>> Yes its strange.
>>>>>
>>>
>>> I have clarified this above. Its not strange but appeared strange
>>> because we were checking wrong conditions.
>>>
>>>
>>>>>> - Marijn
>>
--
With best wishes
Dmitry
On 6/16/2023 5:35 PM, Dmitry Baryshkov wrote:
> On 17/06/2023 00:54, Marijn Suijten wrote:
>> On 2023-06-16 14:18:39, Abhinav Kumar wrote:
>>>
>>>
>>> On 6/14/2023 12:56 AM, Dmitry Baryshkov wrote:
>>>> On 14/06/2023 04:57, Jessica Zhang wrote:
>>>>> Add a DPU INTF op to set the DATABUS_WIDEN register to enable the
>>>>> databus-widen mode datapath.
>>>>>
>>>>> Signed-off-by: Jessica Zhang <[email protected]>
>>>>> ---
>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 3 +++
>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 12
>>>>> ++++++++++++
>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 3 +++
>>>>> 3 files changed, 18 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>>> index b856c6286c85..124ba96bebda 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>>>>> @@ -70,6 +70,9 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>>>> if (intf_cfg.dsc != 0 &&
>>>>> phys_enc->hw_intf->ops.enable_compression)
>>>>>
>>>>> phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
>>>>> +
>>>>> + if (phys_enc->hw_intf->ops.enable_widebus)
>>>>> + phys_enc->hw_intf->ops.enable_widebus(phys_enc->hw_intf);
>>>>
>>>> No. Please provide a single function which takes necessary
>>>> configuration, including compression and wide_bus_enable.
>>>>
>>>
>>> There are two ways to look at this. Your point is coming from the
>>> perspective that its programming the same register but just a different
>>> bit. But that will also make it a bit confusing.
>
> My point is to have a high-level function that configures the INTF for
> the CMD mode. This way it can take a structure with necessary
> configuration bits.
Hi Dmitry,
After discussing this approach with Abhinav, we still have a few
questions about it:
Currently, only 3 of the 32 bits for INTF_CONFIG2 are being used (the
rest are reserved with no plans of being programmed in the future). Does
this still justify the use of a struct to pass in the necessary
configuration?
In addition, it seems that video mode does all its INTF_CONFIG2
configuration separately in dpu_hw_intf_setup_timing_engine(). If we
have a generic set_intf_config2() op, it might be good to have it as
part of a larger cleanup where we have both video and command mode use
the generic op. What are your thoughts on this?
Thanks,
Jessica Zhang
>
>>>
>>> So lets say the function is called intf_cfg2_xxx(..., bool widebus, bool
>>> data_compress)
>>>
>>> Now for the caller who wants to just enable widebus this will be
>>>
>>> intf_cfg2_xxx(....., true, false)
>>>
>>> if we want to do both
>>>
>>> intf_cfg2_xxx(...., true, true)
>>>
>>> the last one where we want to do just data_compress(highly unlikely and
>>> not recommended)
>>>
>>> intf_cfg2_xxx(...., false, true)
>>>
>>> Now someone looking at the code will have to go and find out what each
>>> bool is.
>>>
>>> Whereas with separate ops, its kind of implicit.
>>
>> That's why you never pass bools as function argument (edge-case if it is
>> the only argument, and its meaning becomes clear from the function
>> name). Use enumerations anywhere else.
>>
>> - Marijn
>>
>>>
>>> For that reason, I dont think this patch is bad at all.
>
> --
> With best wishes
> Dmitry
>
On 6/16/2023 5:37 PM, Dmitry Baryshkov wrote:
> On 17/06/2023 00:10, Abhinav Kumar wrote:
>>
>>
>> On 6/14/2023 1:43 PM, Dmitry Baryshkov wrote:
>>> On 14/06/2023 23:39, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 6/14/2023 12:54 PM, Abhinav Kumar wrote:
>>>>>
>>>>>
>>>>> On 6/14/2023 12:35 PM, Abhinav Kumar wrote:
>>>>>>
>>>>>>
>>>>>> On 6/14/2023 5:23 AM, Marijn Suijten wrote:
>>>>>>> On 2023-06-14 15:01:59, Dmitry Baryshkov wrote:
>>>>>>>> On 14/06/2023 14:42, Marijn Suijten wrote:
>>>>>>>>> On 2023-06-13 18:57:11, Jessica Zhang wrote:
>>>>>>>>>> DPU 5.x+ supports a databus widen mode that allows more data
>>>>>>>>>> to be sent
>>>>>>>>>> per pclk. Enable this feature flag on all relevant chipsets.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Jessica Zhang <[email protected]>
>>>>>>>>>> ---
>>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 ++-
>>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
>>>>>>>>>> 2 files changed, 4 insertions(+), 1 deletion(-)
>>>>>>>>>>
>>>>>>>>>> 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 36ba3f58dcdf..0be7bf0bfc41 100644
>>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>>>>>>>> @@ -103,7 +103,8 @@
>>>>>>>>>> (BIT(DPU_INTF_INPUT_CTRL) | \
>>>>>>>>>> BIT(DPU_INTF_TE) | \
>>>>>>>>>> BIT(DPU_INTF_STATUS_SUPPORTED) | \
>>>>>>>>>> - BIT(DPU_DATA_HCTL_EN))
>>>>>>>>>> + BIT(DPU_DATA_HCTL_EN) | \
>>>>>>>>>> + BIT(DPU_INTF_DATABUS_WIDEN))
>>>>>>>>>
>>>>>>>>> This doesn't work. DPU 5.0.0 is SM8150, which has DSI 6G 2.3.
>>>>>>>>> In the
>>>>>>>>> last patch for DSI you state and enable widebus for DSI 6G 2.5+
>>>>>>>>> only,
>>>>>>>>> meaning DPU and DSI are now desynced, and the output is completely
>>>>>>>>> corrupted.
>>>>>>>
>>>>
>>>> I looked at the internal docs and also this change. This change is
>>>> incorrect because this will try to enable widebus for DPU >= 5.0 and
>>>> DSI >= 2.5
>>>>
>>>> That was not the intended right condition as thats not what the docs
>>>> say.
>>>>
>>>> We should enable for DPU >= 7.0 and DSI >= 2.5
>>>>
>>>> Is there any combination where this compatibility is broken? That
>>>> would be the strange thing for me ( not DPU 5.0 and DSI 2.5 as that
>>>> was incorrect)
>>>>
>>>> Part of this confusion is because of catalog macro re-use again.
>>>>
>>>> This series is a good candidate and infact I think we should only do
>>>> core_revision based check on DPU and DSI to avoid bringing the
>>>> catalog mess into this.
>>>
>>> I have just a single request here: can we please have the same
>>> approach for both DSI and DP? I don't mind changing DP code if it
>>> makes it better. If you don't have better reasons, I like the idea of
>>> DSI/DP dictating whether wide bus should be used on the particular
>>> interface. It allows us to handle possible errata or corner cases
>>> there. Another option would be to make DPU tell DSI / DP whether the
>>> wide bus is enabled or not, but I'd say, this is slightly worse
>>> solution.
>>>
>>
>> Today, DP's widebus does not check if DPU supports that or not.
>>
>> DPU encoder queries the DP whether widebus is available and enables it.
>>
>> We can also do the same thing for DSI.
>>
>> So for intf_type of DSI, DPU encoder will query DSI if it supports
>> widebus.
>
> Not if it supports wide bus. But the check is whether enabling wide bus
> is requested by the output driver (DSI/DP).
Hi Dmitry,
Can you explain what you mean by "requested by output driver"? FWIW, if
the DSI version supports wide bus && if DSC is enabled, then wide bus
will be enabled in DSI.
Thanks,
Jessica Zhang
>
>>
>> Then DSI will do its version checks and for DSC it will say yes.
>>
>> This way, we will never check for the DPU's core revision for DSI and
>> purely rely of DP/DSI's hw revisions.
>>
>> Thats fine with me because that way we again just rely on the hw
>> revision to enable the feature.
>>
>> But as a result I am still going to drop this patch which adds widebus
>> to the catalog as a dpu cap which I always wanted to do anyway as we
>> will just rely on the DSI and DP hw revisions.
>
> Yep.
>
>>
>>>>
>>>>>>> Tested this on SM8350 which actually has DSI 2.5, and it is also
>>>>>>> corrupted with this series so something else on this series might be
>>>>>>> broken.
>>>>>>>
>>>>>
>>>>> Missed this response. That seems strange.
>>>>>
>>>>> This series was tested on SM8350 HDK with a command mode panel.
>>>>>
>>>>> We will fix the DPU-DSI handshake and post a v2 but your issue
>>>>> needs investigation in parallel.
>>>>>
>>>>> So another bug to track that would be great.
>>>>>
>>>>>>>>> Is the bound in dsi_host wrong, or do DPU and DSI need to
>>>>>>>>> communicate
>>>>>>>>> when widebus will be enabled, based on DPU && DSI supporting it?
>>>>>>>>
>>>>>>>> I'd prefer to follow the second approach, as we did for DP. DPU
>>>>>>>> asks the
>>>>>>>> actual video output driver if widebus is to be enabled.
>>>>>>>
>>>>>>
>>>>>> I was afraid of this. This series was made on an assumption that
>>>>>> the DPU version of widebus and DSI version of widebus would be
>>>>>> compatible but looks like already SM8150 is an outlier.
>>>>>>
>>>>>> Yes, I think we have to go with second approach.
>>>>>>
>>>>>> DPU queries DSI if it supports widebus and enables it.
>>>>>>
>>>>>> Thanks for your responses. We will post a v2.
>>>>>>
>>>>>>> Doesn't it seem very strange that DPU 5.x+ comes with a widebus
>>>>>>> feature,
>>>>>>> but the DSI does not until two revisions later? Or is this
>>>>>>> available on
>>>>>>> every interface, but only for a different (probably DP) encoder
>>>>>>> block?
>>>>>>>
>>>>>>
>>>>>> Yes its strange.
>>>>>>
>>>>
>>>> I have clarified this above. Its not strange but appeared strange
>>>> because we were checking wrong conditions.
>>>>
>>>>
>>>>>>> - Marijn
>>>
>
> --
> With best wishes
> Dmitry
>
On Wed, 21 Jun 2023 at 00:38, Jessica Zhang <[email protected]> wrote:
>
>
>
> On 6/16/2023 5:35 PM, Dmitry Baryshkov wrote:
> > On 17/06/2023 00:54, Marijn Suijten wrote:
> >> On 2023-06-16 14:18:39, Abhinav Kumar wrote:
> >>>
> >>>
> >>> On 6/14/2023 12:56 AM, Dmitry Baryshkov wrote:
> >>>> On 14/06/2023 04:57, Jessica Zhang wrote:
> >>>>> Add a DPU INTF op to set the DATABUS_WIDEN register to enable the
> >>>>> databus-widen mode datapath.
> >>>>>
> >>>>> Signed-off-by: Jessica Zhang <[email protected]>
> >>>>> ---
> >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 3 +++
> >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 12
> >>>>> ++++++++++++
> >>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 3 +++
> >>>>> 3 files changed, 18 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> >>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> >>>>> index b856c6286c85..124ba96bebda 100644
> >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> >>>>> @@ -70,6 +70,9 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
> >>>>> if (intf_cfg.dsc != 0 &&
> >>>>> phys_enc->hw_intf->ops.enable_compression)
> >>>>>
> >>>>> phys_enc->hw_intf->ops.enable_compression(phys_enc->hw_intf);
> >>>>> +
> >>>>> + if (phys_enc->hw_intf->ops.enable_widebus)
> >>>>> + phys_enc->hw_intf->ops.enable_widebus(phys_enc->hw_intf);
> >>>>
> >>>> No. Please provide a single function which takes necessary
> >>>> configuration, including compression and wide_bus_enable.
> >>>>
> >>>
> >>> There are two ways to look at this. Your point is coming from the
> >>> perspective that its programming the same register but just a different
> >>> bit. But that will also make it a bit confusing.
> >
> > My point is to have a high-level function that configures the INTF for
> > the CMD mode. This way it can take a structure with necessary
> > configuration bits.
>
> Hi Dmitry,
>
> After discussing this approach with Abhinav, we still have a few
> questions about it:
>
> Currently, only 3 of the 32 bits for INTF_CONFIG2 are being used (the
> rest are reserved with no plans of being programmed in the future). Does
> this still justify the use of a struct to pass in the necessary
> configuration?
Yes.
>
> In addition, it seems that video mode does all its INTF_CONFIG2
> configuration separately in dpu_hw_intf_setup_timing_engine(). If we
> have a generic set_intf_config2() op, it might be good to have it as
> part of a larger cleanup where we have both video and command mode use
> the generic op. What are your thoughts on this?
No. If I get your idea right, this would mean moving INTF_CONFIG2
knowledge to the caller, which, I think, is a bad idea.
>
> Thanks,
>
> Jessica Zhang
>
> >
> >>>
> >>> So lets say the function is called intf_cfg2_xxx(..., bool widebus, bool
> >>> data_compress)
> >>>
> >>> Now for the caller who wants to just enable widebus this will be
> >>>
> >>> intf_cfg2_xxx(....., true, false)
> >>>
> >>> if we want to do both
> >>>
> >>> intf_cfg2_xxx(...., true, true)
> >>>
> >>> the last one where we want to do just data_compress(highly unlikely and
> >>> not recommended)
> >>>
> >>> intf_cfg2_xxx(...., false, true)
> >>>
> >>> Now someone looking at the code will have to go and find out what each
> >>> bool is.
> >>>
> >>> Whereas with separate ops, its kind of implicit.
> >>
> >> That's why you never pass bools as function argument (edge-case if it is
> >> the only argument, and its meaning becomes clear from the function
> >> name). Use enumerations anywhere else.
> >>
> >> - Marijn
> >>
> >>>
> >>> For that reason, I dont think this patch is bad at all.
> >
> > --
> > With best wishes
> > Dmitry
> >
--
With best wishes
Dmitry
On Wed, 21 Jun 2023 at 00:37, Jessica Zhang <[email protected]> wrote:
>
>
>
> On 6/16/2023 5:37 PM, Dmitry Baryshkov wrote:
> > On 17/06/2023 00:10, Abhinav Kumar wrote:
> >>
> >>
> >> On 6/14/2023 1:43 PM, Dmitry Baryshkov wrote:
> >>> On 14/06/2023 23:39, Abhinav Kumar wrote:
> >>>>
> >>>>
> >>>> On 6/14/2023 12:54 PM, Abhinav Kumar wrote:
> >>>>>
> >>>>>
> >>>>> On 6/14/2023 12:35 PM, Abhinav Kumar wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 6/14/2023 5:23 AM, Marijn Suijten wrote:
> >>>>>>> On 2023-06-14 15:01:59, Dmitry Baryshkov wrote:
> >>>>>>>> On 14/06/2023 14:42, Marijn Suijten wrote:
> >>>>>>>>> On 2023-06-13 18:57:11, Jessica Zhang wrote:
> >>>>>>>>>> DPU 5.x+ supports a databus widen mode that allows more data
> >>>>>>>>>> to be sent
> >>>>>>>>>> per pclk. Enable this feature flag on all relevant chipsets.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Jessica Zhang <[email protected]>
> >>>>>>>>>> ---
> >>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 ++-
> >>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
> >>>>>>>>>> 2 files changed, 4 insertions(+), 1 deletion(-)
> >>>>>>>>>>
> >>>>>>>>>> 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 36ba3f58dcdf..0be7bf0bfc41 100644
> >>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> >>>>>>>>>> @@ -103,7 +103,8 @@
> >>>>>>>>>> (BIT(DPU_INTF_INPUT_CTRL) | \
> >>>>>>>>>> BIT(DPU_INTF_TE) | \
> >>>>>>>>>> BIT(DPU_INTF_STATUS_SUPPORTED) | \
> >>>>>>>>>> - BIT(DPU_DATA_HCTL_EN))
> >>>>>>>>>> + BIT(DPU_DATA_HCTL_EN) | \
> >>>>>>>>>> + BIT(DPU_INTF_DATABUS_WIDEN))
> >>>>>>>>>
> >>>>>>>>> This doesn't work. DPU 5.0.0 is SM8150, which has DSI 6G 2.3.
> >>>>>>>>> In the
> >>>>>>>>> last patch for DSI you state and enable widebus for DSI 6G 2.5+
> >>>>>>>>> only,
> >>>>>>>>> meaning DPU and DSI are now desynced, and the output is completely
> >>>>>>>>> corrupted.
> >>>>>>>
> >>>>
> >>>> I looked at the internal docs and also this change. This change is
> >>>> incorrect because this will try to enable widebus for DPU >= 5.0 and
> >>>> DSI >= 2.5
> >>>>
> >>>> That was not the intended right condition as thats not what the docs
> >>>> say.
> >>>>
> >>>> We should enable for DPU >= 7.0 and DSI >= 2.5
> >>>>
> >>>> Is there any combination where this compatibility is broken? That
> >>>> would be the strange thing for me ( not DPU 5.0 and DSI 2.5 as that
> >>>> was incorrect)
> >>>>
> >>>> Part of this confusion is because of catalog macro re-use again.
> >>>>
> >>>> This series is a good candidate and infact I think we should only do
> >>>> core_revision based check on DPU and DSI to avoid bringing the
> >>>> catalog mess into this.
> >>>
> >>> I have just a single request here: can we please have the same
> >>> approach for both DSI and DP? I don't mind changing DP code if it
> >>> makes it better. If you don't have better reasons, I like the idea of
> >>> DSI/DP dictating whether wide bus should be used on the particular
> >>> interface. It allows us to handle possible errata or corner cases
> >>> there. Another option would be to make DPU tell DSI / DP whether the
> >>> wide bus is enabled or not, but I'd say, this is slightly worse
> >>> solution.
> >>>
> >>
> >> Today, DP's widebus does not check if DPU supports that or not.
> >>
> >> DPU encoder queries the DP whether widebus is available and enables it.
> >>
> >> We can also do the same thing for DSI.
> >>
> >> So for intf_type of DSI, DPU encoder will query DSI if it supports
> >> widebus.
> >
> > Not if it supports wide bus. But the check is whether enabling wide bus
> > is requested by the output driver (DSI/DP).
>
> Hi Dmitry,
>
> Can you explain what you mean by "requested by output driver"? FWIW, if
> the DSI version supports wide bus && if DSC is enabled, then wide bus
> will be enabled in DSI.
Like for DP, let DSI select whether a wide bus should be enabled or
not, then let DPU get this flag from DSI.
>
> Thanks,
>
> Jessica Zhang
>
> >
> >>
> >> Then DSI will do its version checks and for DSC it will say yes.
> >>
> >> This way, we will never check for the DPU's core revision for DSI and
> >> purely rely of DP/DSI's hw revisions.
> >>
> >> Thats fine with me because that way we again just rely on the hw
> >> revision to enable the feature.
> >>
> >> But as a result I am still going to drop this patch which adds widebus
> >> to the catalog as a dpu cap which I always wanted to do anyway as we
> >> will just rely on the DSI and DP hw revisions.
> >
> > Yep.
> >
> >>
> >>>>
> >>>>>>> Tested this on SM8350 which actually has DSI 2.5, and it is also
> >>>>>>> corrupted with this series so something else on this series might be
> >>>>>>> broken.
> >>>>>>>
> >>>>>
> >>>>> Missed this response. That seems strange.
> >>>>>
> >>>>> This series was tested on SM8350 HDK with a command mode panel.
> >>>>>
> >>>>> We will fix the DPU-DSI handshake and post a v2 but your issue
> >>>>> needs investigation in parallel.
> >>>>>
> >>>>> So another bug to track that would be great.
> >>>>>
> >>>>>>>>> Is the bound in dsi_host wrong, or do DPU and DSI need to
> >>>>>>>>> communicate
> >>>>>>>>> when widebus will be enabled, based on DPU && DSI supporting it?
> >>>>>>>>
> >>>>>>>> I'd prefer to follow the second approach, as we did for DP. DPU
> >>>>>>>> asks the
> >>>>>>>> actual video output driver if widebus is to be enabled.
> >>>>>>>
> >>>>>>
> >>>>>> I was afraid of this. This series was made on an assumption that
> >>>>>> the DPU version of widebus and DSI version of widebus would be
> >>>>>> compatible but looks like already SM8150 is an outlier.
> >>>>>>
> >>>>>> Yes, I think we have to go with second approach.
> >>>>>>
> >>>>>> DPU queries DSI if it supports widebus and enables it.
> >>>>>>
> >>>>>> Thanks for your responses. We will post a v2.
> >>>>>>
> >>>>>>> Doesn't it seem very strange that DPU 5.x+ comes with a widebus
> >>>>>>> feature,
> >>>>>>> but the DSI does not until two revisions later? Or is this
> >>>>>>> available on
> >>>>>>> every interface, but only for a different (probably DP) encoder
> >>>>>>> block?
> >>>>>>>
> >>>>>>
> >>>>>> Yes its strange.
> >>>>>>
> >>>>
> >>>> I have clarified this above. Its not strange but appeared strange
> >>>> because we were checking wrong conditions.
> >>>>
> >>>>
> >>>>>>> - Marijn
> >>>
> >
> > --
> > With best wishes
> > Dmitry
> >
--
With best wishes
Dmitry
On 6/20/2023 3:11 PM, Dmitry Baryshkov wrote:
> On Wed, 21 Jun 2023 at 00:37, Jessica Zhang <[email protected]> wrote:
>>
>>
>>
>> On 6/16/2023 5:37 PM, Dmitry Baryshkov wrote:
>>> On 17/06/2023 00:10, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 6/14/2023 1:43 PM, Dmitry Baryshkov wrote:
>>>>> On 14/06/2023 23:39, Abhinav Kumar wrote:
>>>>>>
>>>>>>
>>>>>> On 6/14/2023 12:54 PM, Abhinav Kumar wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 6/14/2023 12:35 PM, Abhinav Kumar wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 6/14/2023 5:23 AM, Marijn Suijten wrote:
>>>>>>>>> On 2023-06-14 15:01:59, Dmitry Baryshkov wrote:
>>>>>>>>>> On 14/06/2023 14:42, Marijn Suijten wrote:
>>>>>>>>>>> On 2023-06-13 18:57:11, Jessica Zhang wrote:
>>>>>>>>>>>> DPU 5.x+ supports a databus widen mode that allows more data
>>>>>>>>>>>> to be sent
>>>>>>>>>>>> per pclk. Enable this feature flag on all relevant chipsets.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Jessica Zhang <[email protected]>
>>>>>>>>>>>> ---
>>>>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 ++-
>>>>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 ++
>>>>>>>>>>>> 2 files changed, 4 insertions(+), 1 deletion(-)
>>>>>>>>>>>>
>>>>>>>>>>>> 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 36ba3f58dcdf..0be7bf0bfc41 100644
>>>>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>>>>>>>>>> @@ -103,7 +103,8 @@
>>>>>>>>>>>> (BIT(DPU_INTF_INPUT_CTRL) | \
>>>>>>>>>>>> BIT(DPU_INTF_TE) | \
>>>>>>>>>>>> BIT(DPU_INTF_STATUS_SUPPORTED) | \
>>>>>>>>>>>> - BIT(DPU_DATA_HCTL_EN))
>>>>>>>>>>>> + BIT(DPU_DATA_HCTL_EN) | \
>>>>>>>>>>>> + BIT(DPU_INTF_DATABUS_WIDEN))
>>>>>>>>>>>
>>>>>>>>>>> This doesn't work. DPU 5.0.0 is SM8150, which has DSI 6G 2.3.
>>>>>>>>>>> In the
>>>>>>>>>>> last patch for DSI you state and enable widebus for DSI 6G 2.5+
>>>>>>>>>>> only,
>>>>>>>>>>> meaning DPU and DSI are now desynced, and the output is completely
>>>>>>>>>>> corrupted.
>>>>>>>>>
>>>>>>
>>>>>> I looked at the internal docs and also this change. This change is
>>>>>> incorrect because this will try to enable widebus for DPU >= 5.0 and
>>>>>> DSI >= 2.5
>>>>>>
>>>>>> That was not the intended right condition as thats not what the docs
>>>>>> say.
>>>>>>
>>>>>> We should enable for DPU >= 7.0 and DSI >= 2.5
>>>>>>
>>>>>> Is there any combination where this compatibility is broken? That
>>>>>> would be the strange thing for me ( not DPU 5.0 and DSI 2.5 as that
>>>>>> was incorrect)
>>>>>>
>>>>>> Part of this confusion is because of catalog macro re-use again.
>>>>>>
>>>>>> This series is a good candidate and infact I think we should only do
>>>>>> core_revision based check on DPU and DSI to avoid bringing the
>>>>>> catalog mess into this.
>>>>>
>>>>> I have just a single request here: can we please have the same
>>>>> approach for both DSI and DP? I don't mind changing DP code if it
>>>>> makes it better. If you don't have better reasons, I like the idea of
>>>>> DSI/DP dictating whether wide bus should be used on the particular
>>>>> interface. It allows us to handle possible errata or corner cases
>>>>> there. Another option would be to make DPU tell DSI / DP whether the
>>>>> wide bus is enabled or not, but I'd say, this is slightly worse
>>>>> solution.
>>>>>
>>>>
>>>> Today, DP's widebus does not check if DPU supports that or not.
>>>>
>>>> DPU encoder queries the DP whether widebus is available and enables it.
>>>>
>>>> We can also do the same thing for DSI.
>>>>
>>>> So for intf_type of DSI, DPU encoder will query DSI if it supports
>>>> widebus.
>>>
>>> Not if it supports wide bus. But the check is whether enabling wide bus
>>> is requested by the output driver (DSI/DP).
>>
>> Hi Dmitry,
>>
>> Can you explain what you mean by "requested by output driver"? FWIW, if
>> the DSI version supports wide bus && if DSC is enabled, then wide bus
>> will be enabled in DSI.
>
> Like for DP, let DSI select whether a wide bus should be enabled or
> not, then let DPU get this flag from DSI.
Got it -- so basically have DSI do the version check *and* the DSC check
in some `msm_dsi_is_widebus_enabled()` helper and have DPU use that
helper to check if widebus is enabled.
I think that should be fine.
Thanks,
Jessica Zhang
>
>>
>> Thanks,
>>
>> Jessica Zhang
>>
>>>
>>>>
>>>> Then DSI will do its version checks and for DSC it will say yes.
>>>>
>>>> This way, we will never check for the DPU's core revision for DSI and
>>>> purely rely of DP/DSI's hw revisions.
>>>>
>>>> Thats fine with me because that way we again just rely on the hw
>>>> revision to enable the feature.
>>>>
>>>> But as a result I am still going to drop this patch which adds widebus
>>>> to the catalog as a dpu cap which I always wanted to do anyway as we
>>>> will just rely on the DSI and DP hw revisions.
>>>
>>> Yep.
>>>
>>>>
>>>>>>
>>>>>>>>> Tested this on SM8350 which actually has DSI 2.5, and it is also
>>>>>>>>> corrupted with this series so something else on this series might be
>>>>>>>>> broken.
>>>>>>>>>
>>>>>>>
>>>>>>> Missed this response. That seems strange.
>>>>>>>
>>>>>>> This series was tested on SM8350 HDK with a command mode panel.
>>>>>>>
>>>>>>> We will fix the DPU-DSI handshake and post a v2 but your issue
>>>>>>> needs investigation in parallel.
>>>>>>>
>>>>>>> So another bug to track that would be great.
>>>>>>>
>>>>>>>>>>> Is the bound in dsi_host wrong, or do DPU and DSI need to
>>>>>>>>>>> communicate
>>>>>>>>>>> when widebus will be enabled, based on DPU && DSI supporting it?
>>>>>>>>>>
>>>>>>>>>> I'd prefer to follow the second approach, as we did for DP. DPU
>>>>>>>>>> asks the
>>>>>>>>>> actual video output driver if widebus is to be enabled.
>>>>>>>>>
>>>>>>>>
>>>>>>>> I was afraid of this. This series was made on an assumption that
>>>>>>>> the DPU version of widebus and DSI version of widebus would be
>>>>>>>> compatible but looks like already SM8150 is an outlier.
>>>>>>>>
>>>>>>>> Yes, I think we have to go with second approach.
>>>>>>>>
>>>>>>>> DPU queries DSI if it supports widebus and enables it.
>>>>>>>>
>>>>>>>> Thanks for your responses. We will post a v2.
>>>>>>>>
>>>>>>>>> Doesn't it seem very strange that DPU 5.x+ comes with a widebus
>>>>>>>>> feature,
>>>>>>>>> but the DSI does not until two revisions later? Or is this
>>>>>>>>> available on
>>>>>>>>> every interface, but only for a different (probably DP) encoder
>>>>>>>>> block?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Yes its strange.
>>>>>>>>
>>>>>>
>>>>>> I have clarified this above. Its not strange but appeared strange
>>>>>> because we were checking wrong conditions.
>>>>>>
>>>>>>
>>>>>>>>> - Marijn
>>>>>
>>>
>>> --
>>> With best wishes
>>> Dmitry
>>>
>
>
>
> --
> With best wishes
> Dmitry
On 2023-06-20 14:38:34, Jessica Zhang wrote:
<snip>
> >>>>> +??? if (phys_enc->hw_intf->ops.enable_widebus)
> >>>>> +??????? phys_enc->hw_intf->ops.enable_widebus(phys_enc->hw_intf);
> >>>>
> >>>> No. Please provide a single function which takes necessary
> >>>> configuration, including compression and wide_bus_enable.
> >>>>
> >>>
> >>> There are two ways to look at this. Your point is coming from the
> >>> perspective that its programming the same register but just a different
> >>> bit. But that will also make it a bit confusing.
> >
> > My point is to have a high-level function that configures the INTF for
> > the CMD mode. This way it can take a structure with necessary
> > configuration bits.
>
> Hi Dmitry,
>
> After discussing this approach with Abhinav, we still have a few
> questions about it:
>
> Currently, only 3 of the 32 bits for INTF_CONFIG2 are being used (the
> rest are reserved with no plans of being programmed in the future). Does
> this still justify the use of a struct to pass in the necessary
> configuration?
No. The point Dmitry is making is **not** about this concidentally
using the same register, but about adding a common codepath to enable
compression on this hw_intf (regardless of the registers it needs to
touch). Similar to how dpu_hw_intf_setup_timing_engine() programs the
hw_intf - including widebus! - for video-mode.
Or even more generically, have a struct similar to intf_timing_params
that says how the intf needs to be configured - without the caller
knowing about INTF_CONFIG2.
struct dpu_hw_intf_cfg is a very good example of how we can use a single
struct and a single callback to configure multiple registers at once
based on some input parameters.
> In addition, it seems that video mode does all its INTF_CONFIG2
> configuration separately in dpu_hw_intf_setup_timing_engine(). If we
> have a generic set_intf_config2() op, it might be good to have it as
> part of a larger cleanup where we have both video and command mode use
> the generic op. What are your thoughts on this?
Not in that way, but if there is a generic enable_compression() or
configure_compression() callback (or even more generic, similar to
setup_intf_cfg in dpu_hw_ctl) that would work for both video-mode and
command-mode, maybe that is beneficial.
- Marijn
On 21/06/2023 18:17, Marijn Suijten wrote:
> On 2023-06-20 14:38:34, Jessica Zhang wrote:
> <snip>
>>>>>>> + if (phys_enc->hw_intf->ops.enable_widebus)
>>>>>>> + phys_enc->hw_intf->ops.enable_widebus(phys_enc->hw_intf);
>>>>>>
>>>>>> No. Please provide a single function which takes necessary
>>>>>> configuration, including compression and wide_bus_enable.
>>>>>>
>>>>>
>>>>> There are two ways to look at this. Your point is coming from the
>>>>> perspective that its programming the same register but just a different
>>>>> bit. But that will also make it a bit confusing.
>>>
>>> My point is to have a high-level function that configures the INTF for
>>> the CMD mode. This way it can take a structure with necessary
>>> configuration bits.
>>
>> Hi Dmitry,
>>
>> After discussing this approach with Abhinav, we still have a few
>> questions about it:
>>
>> Currently, only 3 of the 32 bits for INTF_CONFIG2 are being used (the
>> rest are reserved with no plans of being programmed in the future). Does
>> this still justify the use of a struct to pass in the necessary
>> configuration?
>
> No. The point Dmitry is making is **not** about this concidentally
> using the same register, but about adding a common codepath to enable
> compression on this hw_intf (regardless of the registers it needs to
> touch).
Actually to setup INTF for CMD stream (which is equal to setting up
compression at this point).
> Similar to how dpu_hw_intf_setup_timing_engine() programs the
> hw_intf - including widebus! - for video-mode.
>
> Or even more generically, have a struct similar to intf_timing_params
> that says how the intf needs to be configured - without the caller
> knowing about INTF_CONFIG2.
>
> struct dpu_hw_intf_cfg is a very good example of how we can use a single
> struct and a single callback to configure multiple registers at once
> based on some input parameters.
>
>> In addition, it seems that video mode does all its INTF_CONFIG2
>> configuration separately in dpu_hw_intf_setup_timing_engine(). If we
>> have a generic set_intf_config2() op, it might be good to have it as
>> part of a larger cleanup where we have both video and command mode use
>> the generic op. What are your thoughts on this?
>
> Not in that way, but if there is a generic enable_compression() or
> configure_compression() callback (or even more generic, similar to
> setup_intf_cfg in dpu_hw_ctl) that would work for both video-mode and
> command-mode, maybe that is beneficial.
I'd rather not do this. Let's just 'setup timing enging' vs 'setup CMD'.
For example, it might also include setting up other INTF parameters for
CMD mode (if anything is required later on).
>
> - Marijn
--
With best wishes
Dmitry
On 2023-06-21 19:36:37, Dmitry Baryshkov wrote:
> On 21/06/2023 18:17, Marijn Suijten wrote:
> > On 2023-06-20 14:38:34, Jessica Zhang wrote:
> > <snip>
> >>>>>>> +??? if (phys_enc->hw_intf->ops.enable_widebus)
> >>>>>>> +??????? phys_enc->hw_intf->ops.enable_widebus(phys_enc->hw_intf);
> >>>>>>
> >>>>>> No. Please provide a single function which takes necessary
> >>>>>> configuration, including compression and wide_bus_enable.
> >>>>>>
> >>>>>
> >>>>> There are two ways to look at this. Your point is coming from the
> >>>>> perspective that its programming the same register but just a different
> >>>>> bit. But that will also make it a bit confusing.
> >>>
> >>> My point is to have a high-level function that configures the INTF for
> >>> the CMD mode. This way it can take a structure with necessary
> >>> configuration bits.
> >>
> >> Hi Dmitry,
> >>
> >> After discussing this approach with Abhinav, we still have a few
> >> questions about it:
> >>
> >> Currently, only 3 of the 32 bits for INTF_CONFIG2 are being used (the
> >> rest are reserved with no plans of being programmed in the future). Does
> >> this still justify the use of a struct to pass in the necessary
> >> configuration?
> >
> > No. The point Dmitry is making is **not** about this concidentally
> > using the same register, but about adding a common codepath to enable
> > compression on this hw_intf (regardless of the registers it needs to
> > touch).
>
> Actually to setup INTF for CMD stream (which is equal to setting up
> compression at this point).
Yup, thaty is what I suggested below ("or even more generically").
> > Similar to how dpu_hw_intf_setup_timing_engine() programs the
> > hw_intf - including widebus! - for video-mode.
> >
> > Or even more generically, have a struct similar to intf_timing_params
> > that says how the intf needs to be configured - without the caller
> > knowing about INTF_CONFIG2.
> >
> > struct dpu_hw_intf_cfg is a very good example of how we can use a single
> > struct and a single callback to configure multiple registers at once
> > based on some input parameters.
> >
> >> In addition, it seems that video mode does all its INTF_CONFIG2
> >> configuration separately in dpu_hw_intf_setup_timing_engine(). If we
> >> have a generic set_intf_config2() op, it might be good to have it as
> >> part of a larger cleanup where we have both video and command mode use
> >> the generic op. What are your thoughts on this?
> >
> > Not in that way, but if there is a generic enable_compression() or
> > configure_compression() callback (or even more generic, similar to
> > setup_intf_cfg in dpu_hw_ctl) that would work for both video-mode and
> > command-mode, maybe that is beneficial.
>
> I'd rather not do this. Let's just 'setup timing enging' vs 'setup CMD'.
> For example, it might also include setting up other INTF parameters for
> CMD mode (if anything is required later on).
Sure, sounds good. hw_intf internally could even have a static function
that deduplicates these "setup" function if there is any.
We could rename setup_timing_engine to setup_video_mode to be more clear
to the reader?
- Marijn
On 6/21/2023 9:36 AM, Dmitry Baryshkov wrote:
> On 21/06/2023 18:17, Marijn Suijten wrote:
>> On 2023-06-20 14:38:34, Jessica Zhang wrote:
>> <snip>
>>>>>>>> + if (phys_enc->hw_intf->ops.enable_widebus)
>>>>>>>> + phys_enc->hw_intf->ops.enable_widebus(phys_enc->hw_intf);
>>>>>>>
>>>>>>> No. Please provide a single function which takes necessary
>>>>>>> configuration, including compression and wide_bus_enable.
>>>>>>>
>>>>>>
>>>>>> There are two ways to look at this. Your point is coming from the
>>>>>> perspective that its programming the same register but just a
>>>>>> different
>>>>>> bit. But that will also make it a bit confusing.
>>>>
>>>> My point is to have a high-level function that configures the INTF for
>>>> the CMD mode. This way it can take a structure with necessary
>>>> configuration bits.
>>>
>>> Hi Dmitry,
>>>
>>> After discussing this approach with Abhinav, we still have a few
>>> questions about it:
>>>
>>> Currently, only 3 of the 32 bits for INTF_CONFIG2 are being used (the
>>> rest are reserved with no plans of being programmed in the future). Does
>>> this still justify the use of a struct to pass in the necessary
>>> configuration?
>>
>> No. The point Dmitry is making is **not** about this concidentally
>> using the same register, but about adding a common codepath to enable
>> compression on this hw_intf (regardless of the registers it needs to
>> touch).
>
> Actually to setup INTF for CMD stream (which is equal to setting up
> compression at this point).
>
Yes it should be setup intf for cmd and not enable compression.
Widebus and compression are different features and we should be able to
control them independently.
We just enable them together for DSI. So a separation is necessary.
But I am still not totally convinced we even need to go down the path
for having an op called setup_intf_cmd() which takes in a struct like
struct dpu_cmd_intf_cfg {
bool data_compress;
bool widebus_en;
};
As we have agreed that we will not touch the video mode timing engine
path, it leaves us with only two bits.
And like I said, its not that these two bits always go together. We want
to be able to control them independently which means that its not
necessary both bits program the same register one by one. We might just
end up programming one of them if we just use widebus.
Thats why I am still leaning on keeping this approach.
>> Similar to how dpu_hw_intf_setup_timing_engine() programs the
>> hw_intf - including widebus! - for video-mode.
>>
>> Or even more generically, have a struct similar to intf_timing_params
>> that says how the intf needs to be configured - without the caller
>> knowing about INTF_CONFIG2.
>>
>> struct dpu_hw_intf_cfg is a very good example of how we can use a single
>> struct and a single callback to configure multiple registers at once
>> based on some input parameters.
>>
>>> In addition, it seems that video mode does all its INTF_CONFIG2
>>> configuration separately in dpu_hw_intf_setup_timing_engine(). If we
>>> have a generic set_intf_config2() op, it might be good to have it as
>>> part of a larger cleanup where we have both video and command mode use
>>> the generic op. What are your thoughts on this?
>>
>> Not in that way, but if there is a generic enable_compression() or
>> configure_compression() callback (or even more generic, similar to
>> setup_intf_cfg in dpu_hw_ctl) that would work for both video-mode and
>> command-mode, maybe that is beneficial.
>
> I'd rather not do this. Let's just 'setup timing enging' vs 'setup CMD'.
> For example, it might also include setting up other INTF parameters for
> CMD mode (if anything is required later on).
>
Agreed on setup CMD but I dont know whether we need a setup CMD at all.
Seems like an overkill.
>>
>> - Marijn
>
On 22/06/2023 02:01, Abhinav Kumar wrote:
>
>
> On 6/21/2023 9:36 AM, Dmitry Baryshkov wrote:
>> On 21/06/2023 18:17, Marijn Suijten wrote:
>>> On 2023-06-20 14:38:34, Jessica Zhang wrote:
>>> <snip>
>>>>>>>>> + if (phys_enc->hw_intf->ops.enable_widebus)
>>>>>>>>> + phys_enc->hw_intf->ops.enable_widebus(phys_enc->hw_intf);
>>>>>>>>
>>>>>>>> No. Please provide a single function which takes necessary
>>>>>>>> configuration, including compression and wide_bus_enable.
>>>>>>>>
>>>>>>>
>>>>>>> There are two ways to look at this. Your point is coming from the
>>>>>>> perspective that its programming the same register but just a
>>>>>>> different
>>>>>>> bit. But that will also make it a bit confusing.
>>>>>
>>>>> My point is to have a high-level function that configures the INTF for
>>>>> the CMD mode. This way it can take a structure with necessary
>>>>> configuration bits.
>>>>
>>>> Hi Dmitry,
>>>>
>>>> After discussing this approach with Abhinav, we still have a few
>>>> questions about it:
>>>>
>>>> Currently, only 3 of the 32 bits for INTF_CONFIG2 are being used (the
>>>> rest are reserved with no plans of being programmed in the future).
>>>> Does
>>>> this still justify the use of a struct to pass in the necessary
>>>> configuration?
>>>
>>> No. The point Dmitry is making is **not** about this concidentally
>>> using the same register, but about adding a common codepath to enable
>>> compression on this hw_intf (regardless of the registers it needs to
>>> touch).
>>
>> Actually to setup INTF for CMD stream (which is equal to setting up
>> compression at this point).
>>
>
> Yes it should be setup intf for cmd and not enable compression.
>
> Widebus and compression are different features and we should be able to
> control them independently.
>
> We just enable them together for DSI. So a separation is necessary.
>
> But I am still not totally convinced we even need to go down the path
> for having an op called setup_intf_cmd() which takes in a struct like
>
> struct dpu_cmd_intf_cfg {
> bool data_compress;
> bool widebus_en;
> };
>
> As we have agreed that we will not touch the video mode timing engine
> path, it leaves us with only two bits.
>
> And like I said, its not that these two bits always go together. We want
> to be able to control them independently which means that its not
> necessary both bits program the same register one by one. We might just
> end up programming one of them if we just use widebus.
>
> Thats why I am still leaning on keeping this approach.
I do not like the idea of having small functions being called between
modules. So, yes there will a config of two booleans, but it is
preferable (and more scalable) compared to separate callbacks.
Not to mention that it allows us to program required registers directly
(by setting values) rather than using RMW cycles and thus depending on
the value being previously programmed to these registers.
>
>>> Similar to how dpu_hw_intf_setup_timing_engine() programs the
>>> hw_intf - including widebus! - for video-mode.
>>>
>>> Or even more generically, have a struct similar to intf_timing_params
>>> that says how the intf needs to be configured - without the caller
>>> knowing about INTF_CONFIG2.
>>>
>>> struct dpu_hw_intf_cfg is a very good example of how we can use a single
>>> struct and a single callback to configure multiple registers at once
>>> based on some input parameters.
>>>
>>>> In addition, it seems that video mode does all its INTF_CONFIG2
>>>> configuration separately in dpu_hw_intf_setup_timing_engine(). If we
>>>> have a generic set_intf_config2() op, it might be good to have it as
>>>> part of a larger cleanup where we have both video and command mode use
>>>> the generic op. What are your thoughts on this?
>>>
>>> Not in that way, but if there is a generic enable_compression() or
>>> configure_compression() callback (or even more generic, similar to
>>> setup_intf_cfg in dpu_hw_ctl) that would work for both video-mode and
>>> command-mode, maybe that is beneficial.
>>
>> I'd rather not do this. Let's just 'setup timing enging' vs 'setup
>> CMD'. For example, it might also include setting up other INTF
>> parameters for CMD mode (if anything is required later on).
>>
>
> Agreed on setup CMD but I dont know whether we need a setup CMD at all.
> Seems like an overkill.
>
>>>
>>> - Marijn
>>
--
With best wishes
Dmitry
On 6/21/2023 4:46 PM, Dmitry Baryshkov wrote:
> On 22/06/2023 02:01, Abhinav Kumar wrote:
>>
>>
>> On 6/21/2023 9:36 AM, Dmitry Baryshkov wrote:
>>> On 21/06/2023 18:17, Marijn Suijten wrote:
>>>> On 2023-06-20 14:38:34, Jessica Zhang wrote:
>>>> <snip>
>>>>>>>>>> + if (phys_enc->hw_intf->ops.enable_widebus)
>>>>>>>>>> +
>>>>>>>>>> phys_enc->hw_intf->ops.enable_widebus(phys_enc->hw_intf);
>>>>>>>>>
>>>>>>>>> No. Please provide a single function which takes necessary
>>>>>>>>> configuration, including compression and wide_bus_enable.
>>>>>>>>>
>>>>>>>>
>>>>>>>> There are two ways to look at this. Your point is coming from the
>>>>>>>> perspective that its programming the same register but just a
>>>>>>>> different
>>>>>>>> bit. But that will also make it a bit confusing.
>>>>>>
>>>>>> My point is to have a high-level function that configures the INTF
>>>>>> for
>>>>>> the CMD mode. This way it can take a structure with necessary
>>>>>> configuration bits.
>>>>>
>>>>> Hi Dmitry,
>>>>>
>>>>> After discussing this approach with Abhinav, we still have a few
>>>>> questions about it:
>>>>>
>>>>> Currently, only 3 of the 32 bits for INTF_CONFIG2 are being used (the
>>>>> rest are reserved with no plans of being programmed in the future).
>>>>> Does
>>>>> this still justify the use of a struct to pass in the necessary
>>>>> configuration?
>>>>
>>>> No. The point Dmitry is making is **not** about this concidentally
>>>> using the same register, but about adding a common codepath to enable
>>>> compression on this hw_intf (regardless of the registers it needs to
>>>> touch).
>>>
>>> Actually to setup INTF for CMD stream (which is equal to setting up
>>> compression at this point).
>>>
>>
>> Yes it should be setup intf for cmd and not enable compression.
>>
>> Widebus and compression are different features and we should be able
>> to control them independently.
>>
>> We just enable them together for DSI. So a separation is necessary.
>>
>> But I am still not totally convinced we even need to go down the path
>> for having an op called setup_intf_cmd() which takes in a struct like
>>
>> struct dpu_cmd_intf_cfg {
>> bool data_compress;
>> bool widebus_en;
>> };
>>
>> As we have agreed that we will not touch the video mode timing engine
>> path, it leaves us with only two bits.
>>
>> And like I said, its not that these two bits always go together. We
>> want to be able to control them independently which means that its not
>> necessary both bits program the same register one by one. We might
>> just end up programming one of them if we just use widebus.
>>
>> Thats why I am still leaning on keeping this approach.
>
> I do not like the idea of having small functions being called between
> modules. So, yes there will a config of two booleans, but it is
> preferable (and more scalable) compared to separate callbacks.
>
I definitely agree with the scalable part and I even cross checked that
the number of usable bitfields of this register is going up from one
chipset to the other although once again that depends on whether we will
use those features.
For that reason I am not opposed to the struct idea.
But there is also another pattern i am seeing which worries me. Usable
bitfields sometimes even reduce. For those cases, if we go with a
pre-defined struct it ends up with redundant members as those bitfields
go away.
With the function op based approach, we just call the op if the feature
bit / core revision.
So I wanted to check once more about the fact that we should consider
not just expansion but also reduction.
> Not to mention that it allows us to program required registers directly
> (by setting values) rather than using RMW cycles and thus depending on
> the value being previously programmed to these registers.
>
This will not change. We will still have to use RMW cycles to preserve
the reset values of some of the fields as those are the right values for
the registers and shouldnt be touched.
>>
>>>> Similar to how dpu_hw_intf_setup_timing_engine() programs the
>>>> hw_intf - including widebus! - for video-mode.
>>>>
>>>> Or even more generically, have a struct similar to intf_timing_params
>>>> that says how the intf needs to be configured - without the caller
>>>> knowing about INTF_CONFIG2.
>>>>
>>>> struct dpu_hw_intf_cfg is a very good example of how we can use a
>>>> single
>>>> struct and a single callback to configure multiple registers at once
>>>> based on some input parameters.
>>>>
>>>>> In addition, it seems that video mode does all its INTF_CONFIG2
>>>>> configuration separately in dpu_hw_intf_setup_timing_engine(). If we
>>>>> have a generic set_intf_config2() op, it might be good to have it as
>>>>> part of a larger cleanup where we have both video and command mode use
>>>>> the generic op. What are your thoughts on this?
>>>>
>>>> Not in that way, but if there is a generic enable_compression() or
>>>> configure_compression() callback (or even more generic, similar to
>>>> setup_intf_cfg in dpu_hw_ctl) that would work for both video-mode and
>>>> command-mode, maybe that is beneficial.
>>>
>>> I'd rather not do this. Let's just 'setup timing enging' vs 'setup
>>> CMD'. For example, it might also include setting up other INTF
>>> parameters for CMD mode (if anything is required later on).
>>>
>>
>> Agreed on setup CMD but I dont know whether we need a setup CMD at all.
>> Seems like an overkill.
>>
>>>>
>>>> - Marijn
>>>
>
On 6/14/2023 3:03 AM, Marijn Suijten wrote:
> On 2023-06-14 10:49:31, Dmitry Baryshkov wrote:
>> On 14/06/2023 04:57, Jessica Zhang wrote:
>>> DSI 6G v2.5.x+ supports a data-bus widen mode that allows DSI to send
>>> 48 bits of compressed data per pclk instead of 24.
>>>
>>> For all chipsets that support this mode, enable it whenever DSC is
>>> enabled as recommend by the hardware programming guide.
>>>
>>> Only enable this for command mode as we are currently unable to validate
>>> it for video mode.
>>>
>>> Signed-off-by: Jessica Zhang <[email protected]>
>>> ---
>>>
>>> Note: The dsi.xml.h changes were generated using the headergen2 script in
>>> envytools [1], but the changes to the copyright and rules-ng-ng source file
>>> paths were dropped.
>>>
>>> [1] https://github.com/freedreno/envytools/
>>>
>>> drivers/gpu/drm/msm/dsi/dsi.xml.h | 1 +
>>> drivers/gpu/drm/msm/dsi/dsi_host.c | 19 ++++++++++++++++++-
>>> 2 files changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h b/drivers/gpu/drm/msm/dsi/dsi.xml.h
>>> index a4a154601114..2a7d980e12c3 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
>>> @@ -664,6 +664,7 @@ static inline uint32_t DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP(enum dsi_rgb_swap v
>>> return ((val) << DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__SHIFT) & DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__MASK;
>>> }
>>> #define DSI_CMD_MODE_MDP_CTRL2_BURST_MODE 0x00010000
>>> +#define DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN 0x00100000
>>>
>>> #define REG_DSI_CMD_MODE_MDP_STREAM2_CTRL 0x000001b8
>>> #define DSI_CMD_MODE_MDP_STREAM2_CTRL_DATA_TYPE__MASK 0x0000003f
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> index 5d7b4409e4e9..1da5238e7105 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> @@ -927,6 +927,9 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>> u32 hdisplay = mode->hdisplay;
>>> u32 wc;
>>> int ret;
>>> + bool widebus_supported = msm_host->cfg_hnd->major == MSM_DSI_VER_MAJOR_6G &&
>>> + msm_host->cfg_hnd->minor >= MSM_DSI_6G_VER_MINOR_V2_5_0;
>>> +
>>>
>>> DBG("");
>>>
>>> @@ -973,8 +976,15 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>> *
>>> * hdisplay will be divided by 3 here to account for the fact
>>> * that DPU sends 3 bytes per pclk cycle to DSI.
>>> + *
>>> + * If widebus is supported, set DATABUS_WIDEN register and divide hdisplay by 6
>>> + * instead of 3
>>
>> This is useless, it is already obvious from the code below. Instead
>> there should be something like "wide bus extends that to 6 bytes per
>> pclk cycle"
>
> Yes please. In general, don't paraphrase the code, but explain _why_ it
> is doing what it does. Saying that the widebus feature doubles the
> bandwidth per pclk tick is much more clear than "divide by 6 instead of
> 3" - we can read that from the code.
>
> Overall comments have been very good so far (such as the original "to
> account for the fact that DPU sends 3 bytes per pclk cycle"), though!
>
>>> */
>>> - hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
>>> + if (!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) && widebus_supported)
>>> + hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 6);
>>> + else
>>> + hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
>>> +
>>> h_total += hdisplay;
>>> ha_end = ha_start + hdisplay;
>>> }
>>> @@ -1027,6 +1037,13 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>>> dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_TOTAL,
>>> DSI_CMD_MDP_STREAM0_TOTAL_H_TOTAL(hdisplay) |
>>> DSI_CMD_MDP_STREAM0_TOTAL_V_TOTAL(mode->vdisplay));
>>> +
>>> + if (msm_host->dsc && widebus_supported) {
>>> + u32 mdp_ctrl2 = dsi_read(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2);
>>> +
>>> + mdp_ctrl2 |= DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN;
>>> + dsi_write(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2, mdp_ctrl2);
>>
>> Is widebus applicable only to the CMD mode, or video mode can employ it too?
>
> The patch description states that it was not tested on video-mode yet,
> so I assume it will. But this should also be highlighted with a comment
> (e.g. /* XXX: Allow for video-mode once tested/fixed */), _especially_
> on the check for MIPI_DSI_MODE_VIDEO above.
>
Sure, we can leave a comment.
> If I understand this correctly DSC is not working for video mode at all
> on these setups, right? Or no-one was able to test it? I'm inclined to
> request dropping these artifical guards to have as little friction as
> possible when someone starts enabling and testing this - and less
> patches removing artificial bounds in the future.
>
Noone was able to test it. Like I have said before, we dont have or have
not brought up any DSI DSC panel with video mode. DP will be the first
to validate the video mode path for DSC so even that time we cannot test
DSI with DSC on video mode.
I think we should find a panel which supports cmd and video mode ( I
believe one of the HDKs does have that ) and bring that one up first to
validate this.
I believe we should keep this checks with the comment you have
suggested. If someone tests it and then removes it, I am comfortable
with that.
Till then, I would rather guard that configuration.
> - Marijn
On 23/06/2023 01:37, Abhinav Kumar wrote:
>
>
> On 6/21/2023 4:46 PM, Dmitry Baryshkov wrote:
>> On 22/06/2023 02:01, Abhinav Kumar wrote:
>>>
>>>
>>> On 6/21/2023 9:36 AM, Dmitry Baryshkov wrote:
>>>> On 21/06/2023 18:17, Marijn Suijten wrote:
>>>>> On 2023-06-20 14:38:34, Jessica Zhang wrote:
>>>>> <snip>
>>>>>>>>>>> + if (phys_enc->hw_intf->ops.enable_widebus)
>>>>>>>>>>> + phys_enc->hw_intf->ops.enable_widebus(phys_enc->hw_intf);
>>>>>>>>>>
>>>>>>>>>> No. Please provide a single function which takes necessary
>>>>>>>>>> configuration, including compression and wide_bus_enable.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> There are two ways to look at this. Your point is coming from the
>>>>>>>>> perspective that its programming the same register but just a
>>>>>>>>> different
>>>>>>>>> bit. But that will also make it a bit confusing.
>>>>>>>
>>>>>>> My point is to have a high-level function that configures the
>>>>>>> INTF for
>>>>>>> the CMD mode. This way it can take a structure with necessary
>>>>>>> configuration bits.
>>>>>>
>>>>>> Hi Dmitry,
>>>>>>
>>>>>> After discussing this approach with Abhinav, we still have a few
>>>>>> questions about it:
>>>>>>
>>>>>> Currently, only 3 of the 32 bits for INTF_CONFIG2 are being used (the
>>>>>> rest are reserved with no plans of being programmed in the
>>>>>> future). Does
>>>>>> this still justify the use of a struct to pass in the necessary
>>>>>> configuration?
>>>>>
>>>>> No. The point Dmitry is making is **not** about this concidentally
>>>>> using the same register, but about adding a common codepath to enable
>>>>> compression on this hw_intf (regardless of the registers it needs to
>>>>> touch).
>>>>
>>>> Actually to setup INTF for CMD stream (which is equal to setting up
>>>> compression at this point).
>>>>
>>>
>>> Yes it should be setup intf for cmd and not enable compression.
>>>
>>> Widebus and compression are different features and we should be able
>>> to control them independently.
>>>
>>> We just enable them together for DSI. So a separation is necessary.
>>>
>>> But I am still not totally convinced we even need to go down the path
>>> for having an op called setup_intf_cmd() which takes in a struct like
>>>
>>> struct dpu_cmd_intf_cfg {
>>> bool data_compress;
>>> bool widebus_en;
>>> };
>>>
>>> As we have agreed that we will not touch the video mode timing engine
>>> path, it leaves us with only two bits.
>>>
>>> And like I said, its not that these two bits always go together. We
>>> want to be able to control them independently which means that its
>>> not necessary both bits program the same register one by one. We
>>> might just end up programming one of them if we just use widebus.
>>>
>>> Thats why I am still leaning on keeping this approach.
>>
>> I do not like the idea of having small functions being called between
>> modules. So, yes there will a config of two booleans, but it is
>> preferable (and more scalable) compared to separate callbacks.
>>
>
> I definitely agree with the scalable part and I even cross checked that
> the number of usable bitfields of this register is going up from one
> chipset to the other although once again that depends on whether we will
> use those features.
>
> For that reason I am not opposed to the struct idea.
>
> But there is also another pattern i am seeing which worries me. Usable
> bitfields sometimes even reduce. For those cases, if we go with a
> pre-defined struct it ends up with redundant members as those bitfields
> go away.
>
> With the function op based approach, we just call the op if the feature
> bit / core revision.
>
> So I wanted to check once more about the fact that we should consider
> not just expansion but also reduction.
As we have to support all generations, there is no actual reduction.
Newer SoCs do not have particular feature/bit, but older ones do. By
having multiple optional ops we just move this knowledge from
ops->complex_callback() to _setup_block_ops(). But more importantly the
caller gets more complicated. Instead of just calling ops->set_me_up(),
it has to check all the optional callbacks and call the one by one.
>
>> Not to mention that it allows us to program required registers
>> directly (by setting values) rather than using RMW cycles and thus
>> depending on the value being previously programmed to these registers.
>>
>
> This will not change. We will still have to use RMW cycles to preserve
> the reset values of some of the fields as those are the right values for
> the registers and shouldnt be touched.
I'd like to point to the dpu_hw_intf_setup_timing_engine(), a close
rival callback, setting up the INTF for video mode. It does not do RMW
cycles, it just writes all the registers.
In the worst case, there will be a single RMW instead of having multiple
of them.
>
>>>
>>>>> Similar to how dpu_hw_intf_setup_timing_engine() programs the
>>>>> hw_intf - including widebus! - for video-mode.
>>>>>
>>>>> Or even more generically, have a struct similar to intf_timing_params
>>>>> that says how the intf needs to be configured - without the caller
>>>>> knowing about INTF_CONFIG2.
>>>>>
>>>>> struct dpu_hw_intf_cfg is a very good example of how we can use a
>>>>> single
>>>>> struct and a single callback to configure multiple registers at once
>>>>> based on some input parameters.
>>>>>
>>>>>> In addition, it seems that video mode does all its INTF_CONFIG2
>>>>>> configuration separately in dpu_hw_intf_setup_timing_engine(). If we
>>>>>> have a generic set_intf_config2() op, it might be good to have it as
>>>>>> part of a larger cleanup where we have both video and command mode
>>>>>> use
>>>>>> the generic op. What are your thoughts on this?
>>>>>
>>>>> Not in that way, but if there is a generic enable_compression() or
>>>>> configure_compression() callback (or even more generic, similar to
>>>>> setup_intf_cfg in dpu_hw_ctl) that would work for both video-mode and
>>>>> command-mode, maybe that is beneficial.
>>>>
>>>> I'd rather not do this. Let's just 'setup timing enging' vs 'setup
>>>> CMD'. For example, it might also include setting up other INTF
>>>> parameters for CMD mode (if anything is required later on).
>>>>
>>>
>>> Agreed on setup CMD but I dont know whether we need a setup CMD at all.
>>> Seems like an overkill.
>>>
>>>>>
>>>>> - Marijn
>>>>
>>
--
With best wishes
Dmitry
On 6/22/2023 4:14 PM, Dmitry Baryshkov wrote:
> On 23/06/2023 01:37, Abhinav Kumar wrote:
>>
>>
>> On 6/21/2023 4:46 PM, Dmitry Baryshkov wrote:
>>> On 22/06/2023 02:01, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 6/21/2023 9:36 AM, Dmitry Baryshkov wrote:
>>>>> On 21/06/2023 18:17, Marijn Suijten wrote:
>>>>>> On 2023-06-20 14:38:34, Jessica Zhang wrote:
>>>>>> <snip>
>>>>>>>>>>>> + if (phys_enc->hw_intf->ops.enable_widebus)
>>>>>>>>>>>> + phys_enc->hw_intf->ops.enable_widebus(phys_enc->hw_intf);
>>>>>>>>>>>
>>>>>>>>>>> No. Please provide a single function which takes necessary
>>>>>>>>>>> configuration, including compression and wide_bus_enable.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> There are two ways to look at this. Your point is coming from the
>>>>>>>>>> perspective that its programming the same register but just a
>>>>>>>>>> different
>>>>>>>>>> bit. But that will also make it a bit confusing.
>>>>>>>>
>>>>>>>> My point is to have a high-level function that configures the
>>>>>>>> INTF for
>>>>>>>> the CMD mode. This way it can take a structure with necessary
>>>>>>>> configuration bits.
>>>>>>>
>>>>>>> Hi Dmitry,
>>>>>>>
>>>>>>> After discussing this approach with Abhinav, we still have a few
>>>>>>> questions about it:
>>>>>>>
>>>>>>> Currently, only 3 of the 32 bits for INTF_CONFIG2 are being used
>>>>>>> (the
>>>>>>> rest are reserved with no plans of being programmed in the
>>>>>>> future). Does
>>>>>>> this still justify the use of a struct to pass in the necessary
>>>>>>> configuration?
>>>>>>
>>>>>> No. The point Dmitry is making is **not** about this concidentally
>>>>>> using the same register, but about adding a common codepath to enable
>>>>>> compression on this hw_intf (regardless of the registers it needs to
>>>>>> touch).
>>>>>
>>>>> Actually to setup INTF for CMD stream (which is equal to setting up
>>>>> compression at this point).
>>>>>
>>>>
>>>> Yes it should be setup intf for cmd and not enable compression.
>>>>
>>>> Widebus and compression are different features and we should be able
>>>> to control them independently.
>>>>
>>>> We just enable them together for DSI. So a separation is necessary.
>>>>
>>>> But I am still not totally convinced we even need to go down the
>>>> path for having an op called setup_intf_cmd() which takes in a
>>>> struct like
>>>>
>>>> struct dpu_cmd_intf_cfg {
>>>> bool data_compress;
>>>> bool widebus_en;
>>>> };
>>>>
>>>> As we have agreed that we will not touch the video mode timing
>>>> engine path, it leaves us with only two bits.
>>>>
>>>> And like I said, its not that these two bits always go together. We
>>>> want to be able to control them independently which means that its
>>>> not necessary both bits program the same register one by one. We
>>>> might just end up programming one of them if we just use widebus.
>>>>
>>>> Thats why I am still leaning on keeping this approach.
>>>
>>> I do not like the idea of having small functions being called between
>>> modules. So, yes there will a config of two booleans, but it is
>>> preferable (and more scalable) compared to separate callbacks.
>>>
>>
>> I definitely agree with the scalable part and I even cross checked
>> that the number of usable bitfields of this register is going up from
>> one chipset to the other although once again that depends on whether
>> we will use those features.
>>
>> For that reason I am not opposed to the struct idea.
>>
>> But there is also another pattern i am seeing which worries me. Usable
>> bitfields sometimes even reduce. For those cases, if we go with a
>> pre-defined struct it ends up with redundant members as those
>> bitfields go away.
>>
>> With the function op based approach, we just call the op if the
>> feature bit / core revision.
>>
>> So I wanted to check once more about the fact that we should consider
>> not just expansion but also reduction.
>
> As we have to support all generations, there is no actual reduction.
> Newer SoCs do not have particular feature/bit, but older ones do. By
> having multiple optional ops we just move this knowledge from
> ops->complex_callback() to _setup_block_ops(). But more importantly the
> caller gets more complicated. Instead of just calling ops->set_me_up(),
> it has to check all the optional callbacks and call the one by one.
>
Alright, I am thinking that perhaps because this register is kind of
unique that its actually controlling a specific setting in the datapath,
downstream also has separate ops for this.
But thats fine, we can go ahead with the struct based approach.
>>
>>> Not to mention that it allows us to program required registers
>>> directly (by setting values) rather than using RMW cycles and thus
>>> depending on the value being previously programmed to these registers.
>>>
>>
>> This will not change. We will still have to use RMW cycles to preserve
>> the reset values of some of the fields as those are the right values
>> for the registers and shouldnt be touched.
>
> I'd like to point to the dpu_hw_intf_setup_timing_engine(), a close
> rival callback, setting up the INTF for video mode. It does not do RMW
> cycles, it just writes all the registers.
>
Yes as it ends up ORing pretty much all the usable bits. So it works fine.
> In the worst case, there will be a single RMW instead of having multiple
> of them.
>
>
>>
>>>>
>>>>>> Similar to how dpu_hw_intf_setup_timing_engine() programs the
>>>>>> hw_intf - including widebus! - for video-mode.
>>>>>>
>>>>>> Or even more generically, have a struct similar to intf_timing_params
>>>>>> that says how the intf needs to be configured - without the caller
>>>>>> knowing about INTF_CONFIG2.
>>>>>>
>>>>>> struct dpu_hw_intf_cfg is a very good example of how we can use a
>>>>>> single
>>>>>> struct and a single callback to configure multiple registers at once
>>>>>> based on some input parameters.
>>>>>>
>>>>>>> In addition, it seems that video mode does all its INTF_CONFIG2
>>>>>>> configuration separately in dpu_hw_intf_setup_timing_engine(). If we
>>>>>>> have a generic set_intf_config2() op, it might be good to have it as
>>>>>>> part of a larger cleanup where we have both video and command
>>>>>>> mode use
>>>>>>> the generic op. What are your thoughts on this?
>>>>>>
>>>>>> Not in that way, but if there is a generic enable_compression() or
>>>>>> configure_compression() callback (or even more generic, similar to
>>>>>> setup_intf_cfg in dpu_hw_ctl) that would work for both video-mode and
>>>>>> command-mode, maybe that is beneficial.
>>>>>
>>>>> I'd rather not do this. Let's just 'setup timing enging' vs 'setup
>>>>> CMD'. For example, it might also include setting up other INTF
>>>>> parameters for CMD mode (if anything is required later on).
>>>>>
>>>>
>>>> Agreed on setup CMD but I dont know whether we need a setup CMD at all.
>>>> Seems like an overkill.
>>>>
>>>>>>
>>>>>> - Marijn
>>>>>
>>>
>
On 6/14/2023 2:56 AM, Marijn Suijten wrote:
> On 2023-06-13 18:57:13, Jessica Zhang wrote:
>> DSI 6G v2.5.x+ supports a data-bus widen mode that allows DSI to send
>> 48 bits of compressed data per pclk instead of 24.
>>
>> For all chipsets that support this mode, enable it whenever DSC is
>> enabled as recommend by the hardware programming guide.
>>
>> Only enable this for command mode as we are currently unable to validate
>> it for video mode.
>>
>> Signed-off-by: Jessica Zhang <[email protected]>
>> ---
>>
>> Note: The dsi.xml.h changes were generated using the headergen2 script in
>> envytools [1], but the changes to the copyright and rules-ng-ng source file
>> paths were dropped.
>>
>> [1] https://github.com/freedreno/envytools/
>
> More interesting would be a link to the Mesa MR upstreaming this
> bitfield to dsi.xml [2] (which I have not found on my own yet).
>
> [2]: https://gitlab.freedesktop.org/mesa/mesa/-/blame/main/src/freedreno/registers/dsi/dsi.xml
>
Thats because we havent submitted a MR yet for this on mesa.
Generally, our team does not have legal permissions yet for mesa MRs
other than mesa drm because we got permissions for the modetest.
Rob/Dmitry, can one of you pls help with the corresponding mesa MR for this?
The xml file change was autogenerated so this patch can go in.
>> drivers/gpu/drm/msm/dsi/dsi.xml.h | 1 +
>> drivers/gpu/drm/msm/dsi/dsi_host.c | 19 ++++++++++++++++++-
>> 2 files changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h b/drivers/gpu/drm/msm/dsi/dsi.xml.h
>> index a4a154601114..2a7d980e12c3 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
>> +++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
>> @@ -664,6 +664,7 @@ static inline uint32_t DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP(enum dsi_rgb_swap v
>> return ((val) << DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__SHIFT) & DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__MASK;
>> }
>> #define DSI_CMD_MODE_MDP_CTRL2_BURST_MODE 0x00010000
>> +#define DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN 0x00100000
>>
>> #define REG_DSI_CMD_MODE_MDP_STREAM2_CTRL 0x000001b8
>> #define DSI_CMD_MODE_MDP_STREAM2_CTRL_DATA_TYPE__MASK 0x0000003f
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index 5d7b4409e4e9..1da5238e7105 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -927,6 +927,9 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>> u32 hdisplay = mode->hdisplay;
>> u32 wc;
>> int ret;
>> + bool widebus_supported = msm_host->cfg_hnd->major == MSM_DSI_VER_MAJOR_6G &&
>> + msm_host->cfg_hnd->minor >= MSM_DSI_6G_VER_MINOR_V2_5_0;
>> +
>>
>> DBG("");
>>
>> @@ -973,8 +976,15 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>> *
>> * hdisplay will be divided by 3 here to account for the fact
>> * that DPU sends 3 bytes per pclk cycle to DSI.
>> + *
>> + * If widebus is supported, set DATABUS_WIDEN register and divide hdisplay by 6
>> + * instead of 3
>
> So this should allow us to divide pclk by 2, or have much lower latency?
> Otherwise it'll tick enough times to transmit the data twice.
>
> Note that I brought up the exact same concerns when you used the 3:1
> ratio from dsi_bpp / dsc_bpp in your pclk reduction patch (instad of the
> number of bits/bytes that DPU sends to DSI per pclk), but no-one has
> replied to my inquiry yet.
>
Ideally yes, we could have done pclk/2 on uncompressed pixels but we are
not going to add support for widebus on DSI without DSC as that is not
recommended in our docs.
So this cannot be done.
We tried our best to respond and explain to all your queries both on the
bug and the patch but i guess it just kept coming :)
I am going to try one more time to explain it in the documentation change.
>> */
>> - hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
>> + if (!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) && widebus_supported)
>> + hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 6);
>> + else
>> + hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
>
> Nit: I wonder if this is more concise when written as:
>
> u32 bytes_per_pclk;
> ...
> if (!video && widebus)
> bytes_per_pclk = 6;
> else
> bytes_per_pclk = 3;
>
> hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc),
> bytes_per_pclk);
>
> That is less duplication, **and** the value becomes self-documenting!
>
Sure, no concerns with making this change.
>> +
>> h_total += hdisplay;
>> ha_end = ha_start + hdisplay;
>> }
>> @@ -1027,6 +1037,13 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>> dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_TOTAL,
>> DSI_CMD_MDP_STREAM0_TOTAL_H_TOTAL(hdisplay) |
>> DSI_CMD_MDP_STREAM0_TOTAL_V_TOTAL(mode->vdisplay));
>> +
>> + if (msm_host->dsc && widebus_supported) {
>
> Can we also support widebus for uncompressed streams (sending 2 pixels
> of bpp=24 per pclk), and if so, is that something you want to add in the
> future (a comment would be nice)?
No, we cannot support widebus on uncompressed streams on DSI so we wont
be adding that.
>
>> + u32 mdp_ctrl2 = dsi_read(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2);
>> +
>> + mdp_ctrl2 |= DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN;
>> + dsi_write(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2, mdp_ctrl2);
>> + }
>
> Same comment as on your BURST_MODE patch (which this'll conflict with):
> does this belong to the timing setup or is it better moved to
> dsi_ctrl_config?
>
> - Marijn
>
>> }
>> }
>>
>>
>> --
>> 2.40.1
>>
On 23/06/2023 03:01, Abhinav Kumar wrote:
>
>
> On 6/14/2023 2:56 AM, Marijn Suijten wrote:
>> On 2023-06-13 18:57:13, Jessica Zhang wrote:
>>> DSI 6G v2.5.x+ supports a data-bus widen mode that allows DSI to send
>>> 48 bits of compressed data per pclk instead of 24.
>>>
>>> For all chipsets that support this mode, enable it whenever DSC is
>>> enabled as recommend by the hardware programming guide.
>>>
>>> Only enable this for command mode as we are currently unable to validate
>>> it for video mode.
>>>
>>> Signed-off-by: Jessica Zhang <[email protected]>
>>> ---
>>>
>>> Note: The dsi.xml.h changes were generated using the headergen2
>>> script in
>>> envytools [1], but the changes to the copyright and rules-ng-ng
>>> source file
>>> paths were dropped.
>>>
>>> [1] https://github.com/freedreno/envytools/
>>
>> More interesting would be a link to the Mesa MR upstreaming this
>> bitfield to dsi.xml [2] (which I have not found on my own yet).
>>
>> [2]:
>> https://gitlab.freedesktop.org/mesa/mesa/-/blame/main/src/freedreno/registers/dsi/dsi.xml
>>
>
> Thats because we havent submitted a MR yet for this on mesa.
>
> Generally, our team does not have legal permissions yet for mesa MRs
> other than mesa drm because we got permissions for the modetest.
>
> Rob/Dmitry, can one of you pls help with the corresponding mesa MR for
> this?
>
> The xml file change was autogenerated so this patch can go in.
Ack, I'll handle it.
--
With best wishes
Dmitry
On 2023-06-22 16:04:30, Abhinav Kumar wrote:
<snip>
> >> Is widebus applicable only to the CMD mode, or video mode can employ it too?
> >
> > The patch description states that it was not tested on video-mode yet,
> > so I assume it will. But this should also be highlighted with a comment
> > (e.g. /* XXX: Allow for video-mode once tested/fixed */), _especially_
> > on the check for MIPI_DSI_MODE_VIDEO above.
>
> Sure, we can leave a comment.
Thanks!
>
> > If I understand this correctly DSC is not working for video mode at all
> > on these setups, right? Or no-one was able to test it? I'm inclined to
> > request dropping these artifical guards to have as little friction as
> > possible when someone starts enabling and testing this - and less
> > patches removing artificial bounds in the future.
> >
>
> Noone was able to test it. Like I have said before, we dont have or have
> not brought up any DSI DSC panel with video mode. DP will be the first
> to validate the video mode path for DSC so even that time we cannot test
> DSI with DSC on video mode.
>
> I think we should find a panel which supports cmd and video mode ( I
> believe one of the HDKs does have that ) and bring that one up first to
> validate this.
>
> I believe we should keep this checks with the comment you have
> suggested. If someone tests it and then removes it, I am comfortable
> with that.
>
> Till then, I would rather guard that configuration.
Sure. On the one hand my suggestion to drop it would be to simplify
DSC video-mode "bring-up" and not put up arbitrary barriers, but for
distinct optional features like widebus it makes sense to keep them
guarded so that a developer can enable them one at a time. I'm just
afraid that them being spread far and wide across the codebase makes it
hard to find all the places where this guard is in place.
Unless it is hopefully one of the current active developers testing
video-mode, because we all know what's where now :)
- Marijn
On 2023-06-22 17:01:34, Abhinav Kumar wrote:
<snip>
> > More interesting would be a link to the Mesa MR upstreaming this
> > bitfield to dsi.xml [2] (which I have not found on my own yet).
> >
> > [2]: https://gitlab.freedesktop.org/mesa/mesa/-/blame/main/src/freedreno/registers/dsi/dsi.xml
> >
>
> Thats because we havent submitted a MR yet for this on mesa.
>
> Generally, our team does not have legal permissions yet for mesa MRs
> other than mesa drm because we got permissions for the modetest.
>
> Rob/Dmitry, can one of you pls help with the corresponding mesa MR for this?
Thanks!
> The xml file change was autogenerated so this patch can go in.
<snip>
> >> *
> >> * hdisplay will be divided by 3 here to account for the fact
> >> * that DPU sends 3 bytes per pclk cycle to DSI.
> >> + *
> >> + * If widebus is supported, set DATABUS_WIDEN register and divide hdisplay by 6
> >> + * instead of 3
> >
> > So this should allow us to divide pclk by 2, or have much lower latency?
> > Otherwise it'll tick enough times to transmit the data twice.
> >
> > Note that I brought up the exact same concerns when you used the 3:1
> > ratio from dsi_bpp / dsc_bpp in your pclk reduction patch (instad of the
> > number of bits/bytes that DPU sends to DSI per pclk), but no-one has
> > replied to my inquiry yet.
> >
>
> Ideally yes, we could have done pclk/2 on uncompressed pixels but we are
> not going to add support for widebus on DSI without DSC as that is not
> recommended in our docs.
No-one here mentioned uncompressed pixels?
None of this code suddenly makes DPU send twice as many pixels/bytes to
the DSI, yet we are enabling a feature that makes the bus twice as wide,
so the clock can be halved *for comressed pixels*?
> So this cannot be done.
>
> We tried our best to respond and explain to all your queries both on the
> bug and the patch but i guess it just kept coming :)
Then send less patches! As long as there is activity on the mailing
list there'll always be questions going back and forth, and I don't
think that's unreasonable.
Unless you want to push patches into mainline without questioning.
> I am going to try one more time to explain it in the documentation change.
Would love to hear, why, for compressed streams, the bus is twice as
wide yet pclk is not reduced to account for that.
<snip>
> > Can we also support widebus for uncompressed streams (sending 2 pixels
> > of bpp=24 per pclk), and if so, is that something you want to add in the
> > future (a comment would be nice)?
>
> No, we cannot support widebus on uncompressed streams on DSI so we wont
> be adding that.
And here we start talking about uncompressed pixels *separately*. Okay,
if it is not supported (e.g. widebus is specific to / reserved for DSC)
it of course makes no sense to add it.
- Marijn
On 6/23/2023 12:19 AM, Marijn Suijten wrote:
> On 2023-06-22 17:01:34, Abhinav Kumar wrote:
> <snip>
>>> More interesting would be a link to the Mesa MR upstreaming this
>>> bitfield to dsi.xml [2] (which I have not found on my own yet).
>>>
>>> [2]: https://gitlab.freedesktop.org/mesa/mesa/-/blame/main/src/freedreno/registers/dsi/dsi.xml
>>>
>>
>> Thats because we havent submitted a MR yet for this on mesa.
>>
>> Generally, our team does not have legal permissions yet for mesa MRs
>> other than mesa drm because we got permissions for the modetest.
>>
>> Rob/Dmitry, can one of you pls help with the corresponding mesa MR for this?
>
> Thanks!
>
>> The xml file change was autogenerated so this patch can go in.
> <snip>
>>>> *
>>>> * hdisplay will be divided by 3 here to account for the fact
>>>> * that DPU sends 3 bytes per pclk cycle to DSI.
>>>> + *
>>>> + * If widebus is supported, set DATABUS_WIDEN register and divide hdisplay by 6
>>>> + * instead of 3
>>>
>>> So this should allow us to divide pclk by 2, or have much lower latency?
>>> Otherwise it'll tick enough times to transmit the data twice.
>>>
>>> Note that I brought up the exact same concerns when you used the 3:1
>>> ratio from dsi_bpp / dsc_bpp in your pclk reduction patch (instad of the
>>> number of bits/bytes that DPU sends to DSI per pclk), but no-one has
>>> replied to my inquiry yet.
>>>
>>
>> Ideally yes, we could have done pclk/2 on uncompressed pixels but we are
>> not going to add support for widebus on DSI without DSC as that is not
>> recommended in our docs.
>
> No-one here mentioned uncompressed pixels?
>
> None of this code suddenly makes DPU send twice as many pixels/bytes to
> the DSI, yet we are enabling a feature that makes the bus twice as wide,
> so the clock can be halved *for comressed pixels*?
>
The concept is quite simple
one pixel per clock for uncompresssed without widebubus
2 pixels per clock for uncompressed with widebus (only enabled for DP
not DSI)
3 bytes worth of data for compressed without widebus
6 bytes worth of data for compressed with widebus
When compression happens, we cannot quantify with pixels as the boundary
is not defined with respect to bytes.
You brought up uncompressed in your below comment so I assumed your
question of /2 was about uncompressed too.
>> So this cannot be done.
>>
>> We tried our best to respond and explain to all your queries both on the
>> bug and the patch but i guess it just kept coming :)
>
> Then send less patches! As long as there is activity on the mailing
> list there'll always be questions going back and forth, and I don't
> think that's unreasonable.
>
> Unless you want to push patches into mainline without questioning.
>
the comments were bordering the line of becoming irrelevant to the
patches like discussing video mode on a command mode patch when we had
explained many many times that we did not validate them.
I dont want to add more comments to this. Lets stop discussing this and
focus more on this patch.
>> I am going to try one more time to explain it in the documentation change.
>
> Would love to hear, why, for compressed streams, the bus is twice as
> wide yet pclk is not reduced to account for that.
>
> <snip>
>>> Can we also support widebus for uncompressed streams (sending 2 pixels
>>> of bpp=24 per pclk), and if so, is that something you want to add in the
>>> future (a comment would be nice)?
>>
>> No, we cannot support widebus on uncompressed streams on DSI so we wont
>> be adding that.
>
> And here we start talking about uncompressed pixels *separately*. Okay,
> if it is not supported (e.g. widebus is specific to / reserved for DSC)
> it of course makes no sense to add it.
>
> - Marijn
On 2023-06-23 10:29:51, Abhinav Kumar wrote:
<snip>
> The concept is quite simple
>
> one pixel per clock for uncompresssed without widebubus
>
> 2 pixels per clock for uncompressed with widebus (only enabled for DP
> not DSI)
>
> 3 bytes worth of data for compressed without widebus
>
> 6 bytes worth of data for compressed with widebus
>
> When compression happens, we cannot quantify with pixels as the boundary
> is not defined with respect to bytes.
>
> You brought up uncompressed in your below comment so I assumed your
> question of /2 was about uncompressed too.
No clue where things are going wrong, but you either avoid or
misunderstand the question.
(Talking exclusively about compressed data here!)
pclk is determined based on the number of bytes.
When widebus is enabled, we transfer twice as many bytes per pclk cycle.
Can pclk be reduced by a factor two, as that should still be enough to
transfer the same amount of bytes when widebus is enabled?
> >> We tried our best to respond and explain to all your queries both on the
> >> bug and the patch but i guess it just kept coming :)
> >
> > Then send less patches! As long as there is activity on the mailing
> > list there'll always be questions going back and forth, and I don't
> > think that's unreasonable.
> >
> > Unless you want to push patches into mainline without questioning.
> >
>
> the comments were bordering the line of becoming irrelevant to the
> patches like discussing video mode on a command mode patch when we had
> explained many many times that we did not validate them.
You(r team) came up with irrelevant video-mode checks in these patches,
and you keep bringing up topics that I did not mention (such as
suddently talking about uncompressed formats above). Stop pretending
there's any nefarious intent here unless you intend to push external
contributors away.
> I dont want to add more comments to this. Lets stop discussing this and
> focus more on this patch.
Perhaps if you answer the question?
- Marijn
On 6/23/2023 1:14 PM, Marijn Suijten wrote:
> On 2023-06-23 10:29:51, Abhinav Kumar wrote:
> <snip>
>> The concept is quite simple
>>
>> one pixel per clock for uncompresssed without widebubus
>>
>> 2 pixels per clock for uncompressed with widebus (only enabled for DP
>> not DSI)
>>
>> 3 bytes worth of data for compressed without widebus
>>
>> 6 bytes worth of data for compressed with widebus
>>
>> When compression happens, we cannot quantify with pixels as the boundary
>> is not defined with respect to bytes.
>>
>> You brought up uncompressed in your below comment so I assumed your
>> question of /2 was about uncompressed too.
>
> No clue where things are going wrong, but you either avoid or
> misunderstand the question.
>
> (Talking exclusively about compressed data here!)
>
> pclk is determined based on the number of bytes.
>
> When widebus is enabled, we transfer twice as many bytes per pclk cycle.
>
> Can pclk be reduced by a factor two, as that should still be enough to
> transfer the same amount of bytes when widebus is enabled?
>
I dont know where the misunderstanding is too.
I already did answer that pclk can be /2 for uncompressed.
But for compressed it will be divided by the compression ration.
What is still missing here. Please clarify.
>>>> We tried our best to respond and explain to all your queries both on the
>>>> bug and the patch but i guess it just kept coming :)
>>>
>>> Then send less patches! As long as there is activity on the mailing
>>> list there'll always be questions going back and forth, and I don't
>>> think that's unreasonable.
>>>
>>> Unless you want to push patches into mainline without questioning.
>>>
>>
>> the comments were bordering the line of becoming irrelevant to the
>> patches like discussing video mode on a command mode patch when we had
>> explained many many times that we did not validate them.
>
> You(r team) came up with irrelevant video-mode checks in these patches,
> and you keep bringing up topics that I did not mention (such as
> suddently talking about uncompressed formats above). Stop pretending
> there's any nefarious intent here unless you intend to push external
> contributors away.
>
There is no nefarious intent. If it was there we would not have spent 2
weeks to answer every question on the bug and explaining every math
about it despite calling our patches hacks.
So either something is still missing in our answers or the questions.
So please ask the question whatever has not been answered one more time.
>> I dont want to add more comments to this. Lets stop discussing this and
>> focus more on this patch.
>
> Perhaps if you answer the question?
>
> - Marijn
On 2023-06-23 13:34:06, Abhinav Kumar wrote:
>
>
> On 6/23/2023 1:14 PM, Marijn Suijten wrote:
> > On 2023-06-23 10:29:51, Abhinav Kumar wrote:
> > <snip>
> >> The concept is quite simple
> >>
> >> one pixel per clock for uncompresssed without widebubus
> >>
> >> 2 pixels per clock for uncompressed with widebus (only enabled for DP
> >> not DSI)
> >>
> >> 3 bytes worth of data for compressed without widebus
> >>
> >> 6 bytes worth of data for compressed with widebus
> >>
> >> When compression happens, we cannot quantify with pixels as the boundary
> >> is not defined with respect to bytes.
> >>
> >> You brought up uncompressed in your below comment so I assumed your
> >> question of /2 was about uncompressed too.
> >
> > No clue where things are going wrong, but you either avoid or
> > misunderstand the question.
> >
> > (Talking exclusively about compressed data here!)
> >
> > pclk is determined based on the number of bytes.
> >
> > When widebus is enabled, we transfer twice as many bytes per pclk cycle.
> >
> > Can pclk be reduced by a factor two, as that should still be enough to
> > transfer the same amount of bytes when widebus is enabled?
> >
>
> I dont know where the misunderstanding is too.
>
> I already did answer that pclk can be /2 for uncompressed.
Except that my question is about compressed.
> But for compressed it will be divided by the compression ration.
The question here is "why exactly"? I am looking for the argument that
justifies pclk being twice as high for the number of bytes we need to
send.
Is that answer: pclk is not only used for the bus between DPU and DSI?
If the answer to that question is yes, then I'd ask what the advantage
is of widebus.
<snip>
Let's leave the rest for what it is.
- Marijn
On 6/23/2023 2:33 PM, Marijn Suijten wrote:
> On 2023-06-23 13:34:06, Abhinav Kumar wrote:
>>
>>
>> On 6/23/2023 1:14 PM, Marijn Suijten wrote:
>>> On 2023-06-23 10:29:51, Abhinav Kumar wrote:
>>> <snip>
>>>> The concept is quite simple
>>>>
>>>> one pixel per clock for uncompresssed without widebubus
>>>>
>>>> 2 pixels per clock for uncompressed with widebus (only enabled for DP
>>>> not DSI)
>>>>
>>>> 3 bytes worth of data for compressed without widebus
>>>>
>>>> 6 bytes worth of data for compressed with widebus
>>>>
>>>> When compression happens, we cannot quantify with pixels as the boundary
>>>> is not defined with respect to bytes.
>>>>
>>>> You brought up uncompressed in your below comment so I assumed your
>>>> question of /2 was about uncompressed too.
>>>
>>> No clue where things are going wrong, but you either avoid or
>>> misunderstand the question.
>>>
>>> (Talking exclusively about compressed data here!)
>>>
>>> pclk is determined based on the number of bytes.
>>>
>>> When widebus is enabled, we transfer twice as many bytes per pclk cycle.
>>>
>>> Can pclk be reduced by a factor two, as that should still be enough to
>>> transfer the same amount of bytes when widebus is enabled?
>>>
>>
>> I dont know where the misunderstanding is too.
>>
>> I already did answer that pclk can be /2 for uncompressed.
>
> Except that my question is about compressed.
>
>> But for compressed it will be divided by the compression ration.
>
> The question here is "why exactly"? I am looking for the argument that
> justifies pclk being twice as high for the number of bytes we need to
> send.
>
Ok I think I finally got your question. So you are asking that "Why
cannot we half the pclk even for the compressed case?"
So pclk / = comp_ratio and then
further on top of that
pclk /= 2 for widebus?
Is that your question?
If so pls see below
> Is that answer: pclk is not only used for the bus between DPU and DSI?
>
> If the answer to that question is yes, then I'd ask what the advantage
> is of widebus.
>
pclk is only used between DPU and DSI.
let me explain the purpose of widebus.
If we take an input of 30bpp, then without widebus DPU can only send 24
bits out.
So the compression ratio which we can achieve would only be 2.4 and not
3 (24/10)
But with widebus, it can output 48bits and now since your source bpp is
only 30bits we can send it out fully and get the 3:1 compression ratio.
So widebus actually makes it possible to get the full compression in the
30bpp case. That would be the benefit of widebus for DSI.
Hope i was able to explain it now.
> <snip>
>
> Let's leave the rest for what it is.
>
> - Marijn
On 6/22/2023 4:37 PM, Abhinav Kumar wrote:
>
>
> On 6/22/2023 4:14 PM, Dmitry Baryshkov wrote:
>> On 23/06/2023 01:37, Abhinav Kumar wrote:
>>>
>>>
>>> On 6/21/2023 4:46 PM, Dmitry Baryshkov wrote:
>>>> On 22/06/2023 02:01, Abhinav Kumar wrote:
>>>>>
>>>>>
>>>>> On 6/21/2023 9:36 AM, Dmitry Baryshkov wrote:
>>>>>> On 21/06/2023 18:17, Marijn Suijten wrote:
>>>>>>> On 2023-06-20 14:38:34, Jessica Zhang wrote:
>>>>>>> <snip>
>>>>>>>>>>>>> + if (phys_enc->hw_intf->ops.enable_widebus)
>>>>>>>>>>>>> + phys_enc->hw_intf->ops.enable_widebus(phys_enc->hw_intf);
>>>>>>>>>>>>
>>>>>>>>>>>> No. Please provide a single function which takes necessary
>>>>>>>>>>>> configuration, including compression and wide_bus_enable.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> There are two ways to look at this. Your point is coming from
>>>>>>>>>>> the
>>>>>>>>>>> perspective that its programming the same register but just a
>>>>>>>>>>> different
>>>>>>>>>>> bit. But that will also make it a bit confusing.
>>>>>>>>>
>>>>>>>>> My point is to have a high-level function that configures the
>>>>>>>>> INTF for
>>>>>>>>> the CMD mode. This way it can take a structure with necessary
>>>>>>>>> configuration bits.
>>>>>>>>
>>>>>>>> Hi Dmitry,
>>>>>>>>
>>>>>>>> After discussing this approach with Abhinav, we still have a few
>>>>>>>> questions about it:
>>>>>>>>
>>>>>>>> Currently, only 3 of the 32 bits for INTF_CONFIG2 are being used
>>>>>>>> (the
>>>>>>>> rest are reserved with no plans of being programmed in the
>>>>>>>> future). Does
>>>>>>>> this still justify the use of a struct to pass in the necessary
>>>>>>>> configuration?
>>>>>>>
>>>>>>> No. The point Dmitry is making is **not** about this concidentally
>>>>>>> using the same register, but about adding a common codepath to
>>>>>>> enable
>>>>>>> compression on this hw_intf (regardless of the registers it needs to
>>>>>>> touch).
>>>>>>
>>>>>> Actually to setup INTF for CMD stream (which is equal to setting
>>>>>> up compression at this point).
>>>>>>
>>>>>
>>>>> Yes it should be setup intf for cmd and not enable compression.
>>>>>
>>>>> Widebus and compression are different features and we should be
>>>>> able to control them independently.
>>>>>
>>>>> We just enable them together for DSI. So a separation is necessary.
>>>>>
>>>>> But I am still not totally convinced we even need to go down the
>>>>> path for having an op called setup_intf_cmd() which takes in a
>>>>> struct like
>>>>>
>>>>> struct dpu_cmd_intf_cfg {
>>>>> bool data_compress;
>>>>> bool widebus_en;
>>>>> };
>>>>>
>>>>> As we have agreed that we will not touch the video mode timing
>>>>> engine path, it leaves us with only two bits.
>>>>>
>>>>> And like I said, its not that these two bits always go together. We
>>>>> want to be able to control them independently which means that its
>>>>> not necessary both bits program the same register one by one. We
>>>>> might just end up programming one of them if we just use widebus.
>>>>>
>>>>> Thats why I am still leaning on keeping this approach.
>>>>
>>>> I do not like the idea of having small functions being called
>>>> between modules. So, yes there will a config of two booleans, but it
>>>> is preferable (and more scalable) compared to separate callbacks.
>>>>
>>>
>>> I definitely agree with the scalable part and I even cross checked
>>> that the number of usable bitfields of this register is going up from
>>> one chipset to the other although once again that depends on whether
>>> we will use those features.
>>>
>>> For that reason I am not opposed to the struct idea.
>>>
>>> But there is also another pattern i am seeing which worries me.
>>> Usable bitfields sometimes even reduce. For those cases, if we go
>>> with a pre-defined struct it ends up with redundant members as those
>>> bitfields go away.
>>>
>>> With the function op based approach, we just call the op if the
>>> feature bit / core revision.
>>>
>>> So I wanted to check once more about the fact that we should consider
>>> not just expansion but also reduction.
>>
>> As we have to support all generations, there is no actual reduction.
>> Newer SoCs do not have particular feature/bit, but older ones do. By
>> having multiple optional ops we just move this knowledge from
>> ops->complex_callback() to _setup_block_ops(). But more importantly
>> the caller gets more complicated. Instead of just calling
>> ops->set_me_up(), it has to check all the optional callbacks and call
>> the one by one.
>>
>
> Alright, I am thinking that perhaps because this register is kind of
> unique that its actually controlling a specific setting in the datapath,
> downstream also has separate ops for this.
>
> But thats fine, we can go ahead with the struct based approach.
>
As data_compress has already landed, let me introduced the struct along
with the core_revision based approach in the core_revision series and
this series will expand that struct to include widebus too.
On 6/29/2023 10:26 AM, Abhinav Kumar wrote:
>
>
> On 6/22/2023 4:37 PM, Abhinav Kumar wrote:
>>
>>
>> On 6/22/2023 4:14 PM, Dmitry Baryshkov wrote:
>>> On 23/06/2023 01:37, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 6/21/2023 4:46 PM, Dmitry Baryshkov wrote:
>>>>> On 22/06/2023 02:01, Abhinav Kumar wrote:
>>>>>>
>>>>>>
>>>>>> On 6/21/2023 9:36 AM, Dmitry Baryshkov wrote:
>>>>>>> On 21/06/2023 18:17, Marijn Suijten wrote:
>>>>>>>> On 2023-06-20 14:38:34, Jessica Zhang wrote:
>>>>>>>> <snip>
>>>>>>>>>>>>>> + if (phys_enc->hw_intf->ops.enable_widebus)
>>>>>>>>>>>>>> + phys_enc->hw_intf->ops.enable_widebus(phys_enc->hw_intf);
>>>>>>>>>>>>>
>>>>>>>>>>>>> No. Please provide a single function which takes necessary
>>>>>>>>>>>>> configuration, including compression and wide_bus_enable.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> There are two ways to look at this. Your point is coming
>>>>>>>>>>>> from the
>>>>>>>>>>>> perspective that its programming the same register but just
>>>>>>>>>>>> a different
>>>>>>>>>>>> bit. But that will also make it a bit confusing.
>>>>>>>>>>
>>>>>>>>>> My point is to have a high-level function that configures the
>>>>>>>>>> INTF for
>>>>>>>>>> the CMD mode. This way it can take a structure with necessary
>>>>>>>>>> configuration bits.
>>>>>>>>>
>>>>>>>>> Hi Dmitry,
>>>>>>>>>
>>>>>>>>> After discussing this approach with Abhinav, we still have a few
>>>>>>>>> questions about it:
>>>>>>>>>
>>>>>>>>> Currently, only 3 of the 32 bits for INTF_CONFIG2 are being
>>>>>>>>> used (the
>>>>>>>>> rest are reserved with no plans of being programmed in the
>>>>>>>>> future). Does
>>>>>>>>> this still justify the use of a struct to pass in the necessary
>>>>>>>>> configuration?
>>>>>>>>
>>>>>>>> No. The point Dmitry is making is **not** about this concidentally
>>>>>>>> using the same register, but about adding a common codepath to
>>>>>>>> enable
>>>>>>>> compression on this hw_intf (regardless of the registers it
>>>>>>>> needs to
>>>>>>>> touch).
>>>>>>>
>>>>>>> Actually to setup INTF for CMD stream (which is equal to setting
>>>>>>> up compression at this point).
>>>>>>>
>>>>>>
>>>>>> Yes it should be setup intf for cmd and not enable compression.
>>>>>>
>>>>>> Widebus and compression are different features and we should be
>>>>>> able to control them independently.
>>>>>>
>>>>>> We just enable them together for DSI. So a separation is necessary.
>>>>>>
>>>>>> But I am still not totally convinced we even need to go down the
>>>>>> path for having an op called setup_intf_cmd() which takes in a
>>>>>> struct like
>>>>>>
>>>>>> struct dpu_cmd_intf_cfg {
>>>>>> bool data_compress;
>>>>>> bool widebus_en;
>>>>>> };
>>>>>>
>>>>>> As we have agreed that we will not touch the video mode timing
>>>>>> engine path, it leaves us with only two bits.
>>>>>>
>>>>>> And like I said, its not that these two bits always go together.
>>>>>> We want to be able to control them independently which means that
>>>>>> its not necessary both bits program the same register one by one.
>>>>>> We might just end up programming one of them if we just use widebus.
>>>>>>
>>>>>> Thats why I am still leaning on keeping this approach.
>>>>>
>>>>> I do not like the idea of having small functions being called
>>>>> between modules. So, yes there will a config of two booleans, but
>>>>> it is preferable (and more scalable) compared to separate callbacks.
>>>>>
>>>>
>>>> I definitely agree with the scalable part and I even cross checked
>>>> that the number of usable bitfields of this register is going up
>>>> from one chipset to the other although once again that depends on
>>>> whether we will use those features.
>>>>
>>>> For that reason I am not opposed to the struct idea.
>>>>
>>>> But there is also another pattern i am seeing which worries me.
>>>> Usable bitfields sometimes even reduce. For those cases, if we go
>>>> with a pre-defined struct it ends up with redundant members as those
>>>> bitfields go away.
>>>>
>>>> With the function op based approach, we just call the op if the
>>>> feature bit / core revision.
>>>>
>>>> So I wanted to check once more about the fact that we should
>>>> consider not just expansion but also reduction.
>>>
>>> As we have to support all generations, there is no actual reduction.
>>> Newer SoCs do not have particular feature/bit, but older ones do. By
>>> having multiple optional ops we just move this knowledge from
>>> ops->complex_callback() to _setup_block_ops(). But more importantly
>>> the caller gets more complicated. Instead of just calling
>>> ops->set_me_up(), it has to check all the optional callbacks and call
>>> the one by one.
>>>
>>
>> Alright, I am thinking that perhaps because this register is kind of
>> unique that its actually controlling a specific setting in the
>> datapath, downstream also has separate ops for this.
>>
>> But thats fine, we can go ahead with the struct based approach.
>>
>
> As data_compress has already landed, let me introduced the struct along
> with the core_revision based approach in the core_revision series and
> this series will expand that struct to include widebus too.
Acked. Will rebase on top of the core_revision series and add widebus to
the config struct.
Thanks,
Jessica Zhang