| Message ID | 20230802-add-widebus-support-v3-1-2661706be001@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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: > > Move the setting of dpu_enc.wide_bus_en to > dpu_encoder_virt_atomic_enable() so that it mirrors the setting of > dpu_enc.dsc. because ... ? > > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) Other than the commit message: Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index d34e684a4178..3dcd37c48aac 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -1194,11 +1194,18 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc, > struct dpu_encoder_virt *dpu_enc = NULL; > int ret = 0; > struct drm_display_mode *cur_mode = NULL; > + struct msm_drm_private *priv = drm_enc->dev->dev_private; > + struct msm_display_info *disp_info; > > dpu_enc = to_dpu_encoder_virt(drm_enc); > + disp_info = &dpu_enc->disp_info; > > 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]]); > + > mutex_lock(&dpu_enc->enc_lock); > cur_mode = &dpu_enc->base.crtc->state->adjusted_mode; > > @@ -2383,10 +2390,6 @@ struct drm_encoder *dpu_encoder_init(struct drm_device *dev, > timer_setup(&dpu_enc->frame_done_timer, > dpu_encoder_frame_done_timeout, 0); > > - 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]]); > - > INIT_DELAYED_WORK(&dpu_enc->delayed_off_work, > dpu_encoder_off_work); > dpu_enc->idle_timeout = IDLE_TIMEOUT; > > -- > 2.41.0 >
I find this title very undescriptive, it doesn't really explain from/to where this move is happening nor why. On 2023-08-02 11:08:48, Jessica Zhang wrote: > Move the setting of dpu_enc.wide_bus_en to > dpu_encoder_virt_atomic_enable() so that it mirrors the setting of > dpu_enc.dsc. mirroring "the setting of dpu_enc.dsc" very much sounds like you are mirroring _its value_, but that is not the case. You are moving the initialization (or just setting, because it could also be overwriting?) to _the same place_ where .dsc is assigned. I am pretty sure that this has a runtime impact which we discussed before (hotplug...?) but the commit message omits that. This is mandatory. - Marijn > > Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index d34e684a4178..3dcd37c48aac 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -1194,11 +1194,18 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc, > struct dpu_encoder_virt *dpu_enc = NULL; > int ret = 0; > struct drm_display_mode *cur_mode = NULL; > + struct msm_drm_private *priv = drm_enc->dev->dev_private; > + struct msm_display_info *disp_info; > > dpu_enc = to_dpu_encoder_virt(drm_enc); > + disp_info = &dpu_enc->disp_info; > > 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]]); > + > mutex_lock(&dpu_enc->enc_lock); > cur_mode = &dpu_enc->base.crtc->state->adjusted_mode; > > @@ -2383,10 +2390,6 @@ struct drm_encoder *dpu_encoder_init(struct drm_device *dev, > timer_setup(&dpu_enc->frame_done_timer, > dpu_encoder_frame_done_timeout, 0); > > - 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]]); > - > INIT_DELAYED_WORK(&dpu_enc->delayed_off_work, > dpu_encoder_off_work); > dpu_enc->idle_timeout = IDLE_TIMEOUT; > > -- > 2.41.0 >
On 8/2/2023 12:32 PM, Marijn Suijten wrote: > I find this title very undescriptive, it doesn't really explain from/to > where this move is happening nor why. > > On 2023-08-02 11:08:48, Jessica Zhang wrote: >> Move the setting of dpu_enc.wide_bus_en to >> dpu_encoder_virt_atomic_enable() so that it mirrors the setting of >> dpu_enc.dsc. > > mirroring "the setting of dpu_enc.dsc" very much sounds like you are > mirroring _its value_, but that is not the case. You are moving the > initialization (or just setting, because it could also be overwriting?) > to _the same place_ where .dsc is assigned. Hi Marijn, Hmm.. got it. Will reword it to "mirror how dpu_enc.dsc is being set" if that makes it clearer. > > I am pretty sure that this has a runtime impact which we discussed > before (hotplug...?) but the commit message omits that. This is > mandatory. I'm assuming the prior discussion you're referring to is with Kuogee on his DSC fix [1]. Unlike DSC, both DSI and DP know if wide bus is enabled upon initialization. The main reasons the setting of the wide_bus_en flag was moved here were 1) to mirror how dpu_enc.dsc was being set (as stated in the commit message) as wide bus is related to DSC, and 2) account for the possibility of DSC for DSI being set during runtime in the future. Thanks, Jessica Zhang [1] https://patchwork.freedesktop.org/patch/543867/ > > - Marijn > >> >> Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> index d34e684a4178..3dcd37c48aac 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> @@ -1194,11 +1194,18 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc, >> struct dpu_encoder_virt *dpu_enc = NULL; >> int ret = 0; >> struct drm_display_mode *cur_mode = NULL; >> + struct msm_drm_private *priv = drm_enc->dev->dev_private; >> + struct msm_display_info *disp_info; >> >> dpu_enc = to_dpu_encoder_virt(drm_enc); >> + disp_info = &dpu_enc->disp_info; >> >> 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]]); >> + >> mutex_lock(&dpu_enc->enc_lock); >> cur_mode = &dpu_enc->base.crtc->state->adjusted_mode; >> >> @@ -2383,10 +2390,6 @@ struct drm_encoder *dpu_encoder_init(struct drm_device *dev, >> timer_setup(&dpu_enc->frame_done_timer, >> dpu_encoder_frame_done_timeout, 0); >> >> - 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]]); >> - >> INIT_DELAYED_WORK(&dpu_enc->delayed_off_work, >> dpu_encoder_off_work); >> dpu_enc->idle_timeout = IDLE_TIMEOUT; >> >> -- >> 2.41.0 >>
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index d34e684a4178..3dcd37c48aac 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1194,11 +1194,18 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc, struct dpu_encoder_virt *dpu_enc = NULL; int ret = 0; struct drm_display_mode *cur_mode = NULL; + struct msm_drm_private *priv = drm_enc->dev->dev_private; + struct msm_display_info *disp_info; dpu_enc = to_dpu_encoder_virt(drm_enc); + disp_info = &dpu_enc->disp_info; 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]]); + mutex_lock(&dpu_enc->enc_lock); cur_mode = &dpu_enc->base.crtc->state->adjusted_mode; @@ -2383,10 +2390,6 @@ struct drm_encoder *dpu_encoder_init(struct drm_device *dev, timer_setup(&dpu_enc->frame_done_timer, dpu_encoder_frame_done_timeout, 0); - 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]]); - INIT_DELAYED_WORK(&dpu_enc->delayed_off_work, dpu_encoder_off_work); dpu_enc->idle_timeout = IDLE_TIMEOUT;
Move the setting of dpu_enc.wide_bus_en to dpu_encoder_virt_atomic_enable() so that it mirrors the setting of dpu_enc.dsc. Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com> --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)