| Message ID | 20230802-add-widebus-support-v3-2-2661706be001@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/msm: Enable widebus for DSI | expand |
On Wed, 2 Aug 2023 at 21:09, Jessica Zhang <quic_jesszhan@quicinc.com> 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 <quic_jesszhan@quicinc.com> > --- > 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 >
On 2023-08-02 11:08:49, Jessica Zhang 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 <quic_jesszhan@quicinc.com> > --- > 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]); This inconsistency really is killing. wide_bus vs widebus, and one function has an is_ while the other does not. > > 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); > + } > > 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, > 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 */ Any clue why these weren't just bool types? These suffix-comments also aren't adhering to the kerneldoc format, or is there a different variant? > }; > > /** > 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; > +} Only this default inline implementation is defined, but the function is declared in this commit. Since there's no real functional implementation yet your commit should clarify that it comes later (in a followup commit in the same series? I can't know because I am reviewing this series linearly from start to finish...) or reorder the patches so that this lack of clarity is circumvented entirely. - Marijn > static inline struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi) > { > return NULL; > > -- > 2.41.0 >
On 8/2/2023 11:20 AM, Dmitry Baryshkov wrote: > On Wed, 2 Aug 2023 at 21:09, Jessica Zhang <quic_jesszhan@quicinc.com> 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 <quic_jesszhan@quicinc.com> >> --- >> 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
On 8/2/2023 12:39 PM, Marijn Suijten wrote: > On 2023-08-02 11:08:49, Jessica Zhang 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 <quic_jesszhan@quicinc.com> >> --- >> 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]); > > This inconsistency really is killing. wide_bus vs widebus, and one > function has an is_ while the other does not. Hi Marijn, Acked. Will change the DSI function name to match DP. > >> >> 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); >> + } >> >> 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, >> 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 */ > > Any clue why these weren't just bool types? These suffix-comments also > aren't adhering to the kerneldoc format, or is there a different > variant? It seems that the `u8` declaration and comment docs were meant to mirror the other dpu_hw_intf_* structs [1] [1] https://elixir.bootlin.com/linux/v6.5-rc5/source/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h#L44 > >> }; >> >> /** >> 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; >> +} > > Only this default inline implementation is defined, but the function is > declared in this commit. Since there's no real functional > implementation yet your commit should clarify that it comes later (in a > followup commit in the same series? I can't know because I am reviewing > this series linearly from start to finish...) or reorder the patches so > that this lack of clarity is circumvented entirely. This was so that there wouldn't be a compiler error in cases where CONFIG_MSM_DSI=n (since DPU support is added before DSI support). Thanks, Jessica Zhang > > - Marijn > >> static inline struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi) >> { >> return NULL; >> >> -- >> 2.41.0 >>
On 08/08/2023 00:40, Jessica Zhang wrote: > > > On 8/2/2023 11:20 AM, Dmitry Baryshkov wrote: >> On Wed, 2 Aug 2023 at 21:09, Jessica Zhang <quic_jesszhan@quicinc.com> >> wrote: >>> 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+. After second consideration, let's ignore this part, as wide bus will only be enabled for DSI / CMD after 7.0. If we ever have SoC that has CMD + wide_bus earlier than 5.0, we can reconsider this code pice. Can you please add a comment that the register itself is present earlier (5.0), but it doesn't have to be programmed since the flags will not be set anyway. > >> >>> 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_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]); 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); + } 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, 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; +} static inline struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi) { return NULL;
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 <quic_jesszhan@quicinc.com> --- 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(-)