On Wed, 2 Aug 2023 at 21:09, Jessica Zhang <[email protected]> wrote:
>
> DPU supports a data-bus widen mode for DSI INTF.
>
> Enable this mode for all supported chipsets if widebus is enabled for DSI.
>
> Signed-off-by: Jessica Zhang <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 ++++++++---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 4 +++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 3 +++
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 1 +
> drivers/gpu/drm/msm/msm_drv.h | 6 +++++-
> 5 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 3dcd37c48aac..de08aad39e15 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1196,15 +1196,20 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
> struct drm_display_mode *cur_mode = NULL;
> struct msm_drm_private *priv = drm_enc->dev->dev_private;
> struct msm_display_info *disp_info;
> + int index;
>
> dpu_enc = to_dpu_encoder_virt(drm_enc);
> disp_info = &dpu_enc->disp_info;
>
> + disp_info = &dpu_enc->disp_info;
> + index = disp_info->h_tile_instance[0];
> +
> dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc);
>
> - if (disp_info->intf_type == INTF_DP)
> - dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
> - priv->dp[disp_info->h_tile_instance[0]]);
> + if (disp_info->intf_type == INTF_DSI)
> + dpu_enc->wide_bus_en = msm_dsi_is_widebus_enabled(priv->dsi[index]);
> + else if (disp_info->intf_type == INTF_DP)
> + dpu_enc->wide_bus_en = msm_dp_wide_bus_available(priv->dp[index]);
If you change the order, you won't have to touch DP lines.
>
> mutex_lock(&dpu_enc->enc_lock);
> cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
> 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 df88358e7037..dace6168be2d 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
> @@ -69,8 +69,10 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
> phys_enc->hw_intf,
> phys_enc->hw_pp->idx);
>
> - if (intf_cfg.dsc != 0)
> + if (intf_cfg.dsc != 0) {
> cmd_mode_cfg.data_compress = true;
> + cmd_mode_cfg.wide_bus_en = dpu_encoder_is_widebus_enabled(phys_enc->parent);
> + }
This embeds the knowledge that a wide bus can only be enabled when DSC
is in use. Please move the wide_bus_en assignment out of conditional
code.
>
> if (phys_enc->hw_intf->ops.program_intf_cmd_cfg)
> phys_enc->hw_intf->ops.program_intf_cmd_cfg(phys_enc->hw_intf, &cmd_mode_cfg);
> 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 8ec6505d9e78..dc6f3febb574 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -521,6 +521,9 @@ static void dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx,
This function is only enabled for DPU >= 7.0, while IIRC wide bus can
be enabled even for some of the earlier chipsets.
> if (cmd_mode_cfg->data_compress)
> intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>
> + if (cmd_mode_cfg->wide_bus_en)
> + intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
> +
> DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
> }
>
> 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 77f80531782b..c539025c418b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> @@ -50,6 +50,7 @@ struct dpu_hw_intf_status {
>
> struct dpu_hw_intf_cmd_mode_cfg {
> u8 data_compress; /* enable data compress between dpu and dsi */
> + u8 wide_bus_en; /* enable databus widen mode */
> };
>
> /**
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 9d9d5e009163..e4f706b16aad 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -344,6 +344,7 @@ void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi
> bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi);
> bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi);
> bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi);
> +bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi);
> struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi);
> #else
> static inline void __init msm_dsi_register(void)
> @@ -373,7 +374,10 @@ static inline bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi)
> {
> return false;
> }
> -
> +static inline bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi)
> +{
> + return false;
> +}
Empty line, please.
> static inline struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi)
> {
> return NULL;
>
> --
> 2.41.0
>
--
With best wishes
Dmitry
On 8/2/2023 11:20 AM, Dmitry Baryshkov wrote:
> On Wed, 2 Aug 2023 at 21:09, Jessica Zhang <[email protected]> wrote:
>>
>> DPU supports a data-bus widen mode for DSI INTF.
>>
>> Enable this mode for all supported chipsets if widebus is enabled for DSI.
>>
>> Signed-off-by: Jessica Zhang <[email protected]>
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 ++++++++---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 4 +++-
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 3 +++
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 1 +
>> drivers/gpu/drm/msm/msm_drv.h | 6 +++++-
>> 5 files changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 3dcd37c48aac..de08aad39e15 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -1196,15 +1196,20 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
>> struct drm_display_mode *cur_mode = NULL;
>> struct msm_drm_private *priv = drm_enc->dev->dev_private;
>> struct msm_display_info *disp_info;
>> + int index;
>>
>> dpu_enc = to_dpu_encoder_virt(drm_enc);
>> disp_info = &dpu_enc->disp_info;
>>
>> + disp_info = &dpu_enc->disp_info;
>> + index = disp_info->h_tile_instance[0];
>> +
>> dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc);
>>
>> - if (disp_info->intf_type == INTF_DP)
>> - dpu_enc->wide_bus_en = msm_dp_wide_bus_available(
>> - priv->dp[disp_info->h_tile_instance[0]]);
>> + if (disp_info->intf_type == INTF_DSI)
>> + dpu_enc->wide_bus_en = msm_dsi_is_widebus_enabled(priv->dsi[index]);
>> + else if (disp_info->intf_type == INTF_DP)
>> + dpu_enc->wide_bus_en = msm_dp_wide_bus_available(priv->dp[index]);
>
> If you change the order, you won't have to touch DP lines.
Hi Dmitry,
Acked.
>
>>
>> mutex_lock(&dpu_enc->enc_lock);
>> cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
>> 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 df88358e7037..dace6168be2d 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
>> @@ -69,8 +69,10 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
>> phys_enc->hw_intf,
>> phys_enc->hw_pp->idx);
>>
>> - if (intf_cfg.dsc != 0)
>> + if (intf_cfg.dsc != 0) {
>> cmd_mode_cfg.data_compress = true;
>> + cmd_mode_cfg.wide_bus_en = dpu_encoder_is_widebus_enabled(phys_enc->parent);
>> + }
>
> This embeds the knowledge that a wide bus can only be enabled when DSC
> is in use. Please move the wide_bus_en assignment out of conditional
> code.
Wide bus for DSI will only be enabled if DSC is enabled, so this is
technically not wrong, as DP will use the video mode path.
>
>>
>> if (phys_enc->hw_intf->ops.program_intf_cmd_cfg)
>> phys_enc->hw_intf->ops.program_intf_cmd_cfg(phys_enc->hw_intf, &cmd_mode_cfg);
>> 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 8ec6505d9e78..dc6f3febb574 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
>> @@ -521,6 +521,9 @@ static void dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx,
>
> This function is only enabled for DPU >= 7.0, while IIRC wide bus can
> be enabled even for some of the earlier chipsets.
The command mode path is only called for DSI, which only supports wide
bus for DPU 7.0+.
>
>> if (cmd_mode_cfg->data_compress)
>> intf_cfg2 |= INTF_CFG2_DCE_DATA_COMPRESS;
>>
>> + if (cmd_mode_cfg->wide_bus_en)
>> + intf_cfg2 |= INTF_CFG2_DATABUS_WIDEN;
>> +
>> DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
>> }
>>
>> 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 77f80531782b..c539025c418b 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
>> @@ -50,6 +50,7 @@ struct dpu_hw_intf_status {
>>
>> struct dpu_hw_intf_cmd_mode_cfg {
>> u8 data_compress; /* enable data compress between dpu and dsi */
>> + u8 wide_bus_en; /* enable databus widen mode */
>> };
>>
>> /**
>> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
>> index 9d9d5e009163..e4f706b16aad 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.h
>> +++ b/drivers/gpu/drm/msm/msm_drv.h
>> @@ -344,6 +344,7 @@ void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi
>> bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi);
>> bool msm_dsi_is_bonded_dsi(struct msm_dsi *msm_dsi);
>> bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi);
>> +bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi);
>> struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi);
>> #else
>> static inline void __init msm_dsi_register(void)
>> @@ -373,7 +374,10 @@ static inline bool msm_dsi_is_master_dsi(struct msm_dsi *msm_dsi)
>> {
>> return false;
>> }
>> -
>> +static inline bool msm_dsi_is_widebus_enabled(struct msm_dsi *msm_dsi)
>> +{
>> + return false;
>> +}
>
> Empty line, please.
Acked.
Thanks,
Jessica Zhang
>
>> static inline struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi)
>> {
>> return NULL;
>>
>> --
>> 2.41.0
>>
>
>
> --
> With best wishes
> Dmitry