| Message ID | 20240125193834.7065-15-quic_parellan@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for CDM over DP | expand |
On 25/01/2024 21:38, Paloma Arellano wrote: > Adjust the encoder format programming in the case of video mode for DP > to accommodate CDM related changes. > > Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 16 +++++++++ > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 +++++ > .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 35 ++++++++++++++++--- > drivers/gpu/drm/msm/dp/dp_display.c | 12 +++++++ > drivers/gpu/drm/msm/msm_drv.h | 9 ++++- > 5 files changed, 75 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 b0896814c1562..99ec53446ad21 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -222,6 +222,22 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = { > 15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10 > }; > > +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, const struct drm_display_mode *mode) > +{ > + const struct dpu_encoder_virt *dpu_enc; > + const struct msm_display_info *disp_info; > + struct msm_drm_private *priv; > + > + dpu_enc = to_dpu_encoder_virt(drm_enc); > + disp_info = &dpu_enc->disp_info; > + priv = drm_enc->dev->dev_private; > + > + if (disp_info->intf_type == INTF_DP && > + msm_dp_is_yuv_420_enabled(priv->dp[disp_info->h_tile_instance[0]], mode)) This should not require interacting with DP. If we got here, we must be sure that 4:2:0 is supported and can be configured. > + return DRM_FORMAT_YUV420; > + > + return DRM_FORMAT_RGB888; > +} > > bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc) > { > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > index 7b4afa71f1f96..62255d0aa4487 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > @@ -162,6 +162,14 @@ int dpu_encoder_get_vsync_count(struct drm_encoder *drm_enc); > */ > bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc); > > +/** > + * dpu_encoder_get_drm_fmt - return DRM fourcc format > + * @drm_enc: Pointer to previously created drm encoder structure > + * @mode: Corresponding drm_display_mode for dpu encoder > + */ > +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, > + const struct drm_display_mode *mode); > + > /** > * dpu_encoder_get_crc_values_cnt - get number of physical encoders contained > * in virtual encoder that can collect CRC values > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > index e284bf448bdda..a1dde0ff35dc8 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > @@ -234,6 +234,7 @@ static void dpu_encoder_phys_vid_setup_timing_engine( > { > struct drm_display_mode mode; > struct dpu_hw_intf_timing_params timing_params = { 0 }; > + struct dpu_hw_cdm *hw_cdm; > const struct dpu_format *fmt = NULL; > u32 fmt_fourcc = DRM_FORMAT_RGB888; > unsigned long lock_flags; > @@ -254,17 +255,26 @@ static void dpu_encoder_phys_vid_setup_timing_engine( > DPU_DEBUG_VIDENC(phys_enc, "enabling mode:\n"); > drm_mode_debug_printmodeline(&mode); > > - if (phys_enc->split_role != ENC_ROLE_SOLO) { > + hw_cdm = phys_enc->hw_cdm; > + if (hw_cdm) { > + intf_cfg.cdm = hw_cdm->idx; > + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, &mode); > + } > + > + if (phys_enc->split_role != ENC_ROLE_SOLO || > + dpu_encoder_get_drm_fmt(phys_enc->parent, &mode) == DRM_FORMAT_YUV420) { > mode.hdisplay >>= 1; > mode.htotal >>= 1; > mode.hsync_start >>= 1; > mode.hsync_end >>= 1; > + mode.hskew >>= 1; Separate patch. > > DPU_DEBUG_VIDENC(phys_enc, > - "split_role %d, halve horizontal %d %d %d %d\n", > + "split_role %d, halve horizontal %d %d %d %d %d\n", > phys_enc->split_role, > mode.hdisplay, mode.htotal, > - mode.hsync_start, mode.hsync_end); > + mode.hsync_start, mode.hsync_end, > + mode.hskew); > } > > drm_mode_to_intf_timing_params(phys_enc, &mode, &timing_params); > @@ -412,8 +422,15 @@ static int dpu_encoder_phys_vid_control_vblank_irq( > static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys *phys_enc) > { > struct dpu_hw_ctl *ctl; > + struct dpu_hw_cdm *hw_cdm; > + const struct dpu_format *fmt = NULL; > + u32 fmt_fourcc = DRM_FORMAT_RGB888; > > ctl = phys_enc->hw_ctl; > + hw_cdm = phys_enc->hw_cdm; > + if (hw_cdm) > + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, &phys_enc->cached_mode); > + fmt = dpu_get_dpu_format(fmt_fourcc); > > DPU_DEBUG_VIDENC(phys_enc, "\n"); > > @@ -422,6 +439,8 @@ static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys *phys_enc) > > dpu_encoder_helper_split_config(phys_enc, phys_enc->hw_intf->idx); > > + dpu_encoder_helper_phys_setup_cdm(phys_enc, fmt, CDM_CDWN_OUTPUT_HDMI); If there is no CDM, why do we need to call this? > + > dpu_encoder_phys_vid_setup_timing_engine(phys_enc); > > /* > @@ -437,7 +456,15 @@ static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys *phys_enc) > if (ctl->ops.update_pending_flush_merge_3d && phys_enc->hw_pp->merge_3d) > ctl->ops.update_pending_flush_merge_3d(ctl, phys_enc->hw_pp->merge_3d->idx); > > - if (ctl->ops.update_pending_flush_periph && phys_enc->hw_intf->cap->type == INTF_DP) > + if (ctl->ops.update_pending_flush_cdm && phys_enc->hw_cdm) > + ctl->ops.update_pending_flush_cdm(ctl, hw_cdm->idx); > + > + /* > + * Peripheral flush must be updated whenever flushing SDP packets is needed. > + * SDP packets are required for any YUV format (YUV420, YUV422, YUV444). > + */ > + if (ctl->ops.update_pending_flush_periph && phys_enc->hw_intf->cap->type == INTF_DP && > + phys_enc->hw_cdm) > ctl->ops.update_pending_flush_periph(ctl, phys_enc->hw_intf->idx); Should there be a flush if we are switching from YUV 420 to RGB mode? Also, I'd say, we should move update_pending_flush_periph invocation to this patch. > > skip_flush: > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > index 6d764f5b08727..4329435518351 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -1399,6 +1399,18 @@ void __exit msm_dp_unregister(void) > platform_driver_unregister(&dp_display_driver); > } > > +bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display, > + const struct drm_display_mode *mode) > +{ > + struct dp_display_private *dp; > + const struct drm_display_info *info; > + > + dp = container_of(dp_display, struct dp_display_private, dp_display); > + info = &dp_display->connector->display_info; > + > + return dp_panel_vsc_sdp_supported(dp->panel) && drm_mode_is_420_only(info, mode); YUV 420 modes should be filtered out in mode_valid if VSC SDP is not supported. > +} > + > bool msm_dp_wide_bus_available(const struct msm_dp *dp_display) > { > struct dp_display_private *dp; > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h > index 16a7cbc0b7dd8..b9581bd934e9e 100644 > --- a/drivers/gpu/drm/msm/msm_drv.h > +++ b/drivers/gpu/drm/msm/msm_drv.h > @@ -387,7 +387,8 @@ void __exit msm_dp_unregister(void); > int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, > struct drm_encoder *encoder); > void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_display); > - > +bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display, > + const struct drm_display_mode *mode); > bool msm_dp_wide_bus_available(const struct msm_dp *dp_display); > > #else > @@ -409,6 +410,12 @@ static inline void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm > { > } > > +static inline bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display, > + const struct drm_display_mode *mode) > +{ > + return false; > +} > + > static inline bool msm_dp_wide_bus_available(const struct msm_dp *dp_display) > { > return false;
On 1/25/2024 1:57 PM, Dmitry Baryshkov wrote: > On 25/01/2024 21:38, Paloma Arellano wrote: >> Adjust the encoder format programming in the case of video mode for DP >> to accommodate CDM related changes. >> >> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 16 +++++++++ >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 +++++ >> .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 35 ++++++++++++++++--- >> drivers/gpu/drm/msm/dp/dp_display.c | 12 +++++++ >> drivers/gpu/drm/msm/msm_drv.h | 9 ++++- >> 5 files changed, 75 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 b0896814c1562..99ec53446ad21 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> @@ -222,6 +222,22 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = { >> 15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10 >> }; >> +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, >> const struct drm_display_mode *mode) >> +{ >> + const struct dpu_encoder_virt *dpu_enc; >> + const struct msm_display_info *disp_info; >> + struct msm_drm_private *priv; >> + >> + dpu_enc = to_dpu_encoder_virt(drm_enc); >> + disp_info = &dpu_enc->disp_info; >> + priv = drm_enc->dev->dev_private; >> + >> + if (disp_info->intf_type == INTF_DP && >> + msm_dp_is_yuv_420_enabled(priv->dp[disp_info->h_tile_instance[0]], >> mode)) > > This should not require interacting with DP. If we got here, we must > be sure that 4:2:0 is supported and can be configured. Ack. Will drop this function and only check for if the mode is YUV420. > >> + return DRM_FORMAT_YUV420; >> + >> + return DRM_FORMAT_RGB888; >> +} >> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder >> *drm_enc) >> { >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >> index 7b4afa71f1f96..62255d0aa4487 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >> @@ -162,6 +162,14 @@ int dpu_encoder_get_vsync_count(struct >> drm_encoder *drm_enc); >> */ >> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder >> *drm_enc); >> +/** >> + * dpu_encoder_get_drm_fmt - return DRM fourcc format >> + * @drm_enc: Pointer to previously created drm encoder structure >> + * @mode: Corresponding drm_display_mode for dpu encoder >> + */ >> +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, >> + const struct drm_display_mode *mode); >> + >> /** >> * dpu_encoder_get_crc_values_cnt - get number of physical encoders >> contained >> * in virtual encoder that can collect CRC values >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c >> index e284bf448bdda..a1dde0ff35dc8 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c >> @@ -234,6 +234,7 @@ static void >> dpu_encoder_phys_vid_setup_timing_engine( >> { >> struct drm_display_mode mode; >> struct dpu_hw_intf_timing_params timing_params = { 0 }; >> + struct dpu_hw_cdm *hw_cdm; >> const struct dpu_format *fmt = NULL; >> u32 fmt_fourcc = DRM_FORMAT_RGB888; >> unsigned long lock_flags; >> @@ -254,17 +255,26 @@ static void >> dpu_encoder_phys_vid_setup_timing_engine( >> DPU_DEBUG_VIDENC(phys_enc, "enabling mode:\n"); >> drm_mode_debug_printmodeline(&mode); >> - if (phys_enc->split_role != ENC_ROLE_SOLO) { >> + hw_cdm = phys_enc->hw_cdm; >> + if (hw_cdm) { >> + intf_cfg.cdm = hw_cdm->idx; >> + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, &mode); >> + } >> + >> + if (phys_enc->split_role != ENC_ROLE_SOLO || >> + dpu_encoder_get_drm_fmt(phys_enc->parent, &mode) == >> DRM_FORMAT_YUV420) { >> mode.hdisplay >>= 1; >> mode.htotal >>= 1; >> mode.hsync_start >>= 1; >> mode.hsync_end >>= 1; >> + mode.hskew >>= 1; > > Separate patch. Ack. > >> DPU_DEBUG_VIDENC(phys_enc, >> - "split_role %d, halve horizontal %d %d %d %d\n", >> + "split_role %d, halve horizontal %d %d %d %d %d\n", >> phys_enc->split_role, >> mode.hdisplay, mode.htotal, >> - mode.hsync_start, mode.hsync_end); >> + mode.hsync_start, mode.hsync_end, >> + mode.hskew); >> } >> drm_mode_to_intf_timing_params(phys_enc, &mode, &timing_params); >> @@ -412,8 +422,15 @@ static int dpu_encoder_phys_vid_control_vblank_irq( >> static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys >> *phys_enc) >> { >> struct dpu_hw_ctl *ctl; >> + struct dpu_hw_cdm *hw_cdm; >> + const struct dpu_format *fmt = NULL; >> + u32 fmt_fourcc = DRM_FORMAT_RGB888; >> ctl = phys_enc->hw_ctl; >> + hw_cdm = phys_enc->hw_cdm; >> + if (hw_cdm) >> + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, >> &phys_enc->cached_mode); >> + fmt = dpu_get_dpu_format(fmt_fourcc); >> DPU_DEBUG_VIDENC(phys_enc, "\n"); >> @@ -422,6 +439,8 @@ static void dpu_encoder_phys_vid_enable(struct >> dpu_encoder_phys *phys_enc) >> dpu_encoder_helper_split_config(phys_enc, >> phys_enc->hw_intf->idx); >> + dpu_encoder_helper_phys_setup_cdm(phys_enc, fmt, >> CDM_CDWN_OUTPUT_HDMI); > > If there is no CDM, why do we need to call this? Inside of dpu_encoder_helper_phys_setup_cdm(), there's a check to see if there is a hw_cdm. If there is not, then it immediately exits the function. > >> + >> dpu_encoder_phys_vid_setup_timing_engine(phys_enc); >> /* >> @@ -437,7 +456,15 @@ static void dpu_encoder_phys_vid_enable(struct >> dpu_encoder_phys *phys_enc) >> if (ctl->ops.update_pending_flush_merge_3d && >> phys_enc->hw_pp->merge_3d) >> ctl->ops.update_pending_flush_merge_3d(ctl, >> phys_enc->hw_pp->merge_3d->idx); >> - if (ctl->ops.update_pending_flush_periph && >> phys_enc->hw_intf->cap->type == INTF_DP) >> + if (ctl->ops.update_pending_flush_cdm && phys_enc->hw_cdm) >> + ctl->ops.update_pending_flush_cdm(ctl, hw_cdm->idx); >> + >> + /* >> + * Peripheral flush must be updated whenever flushing SDP >> packets is needed. >> + * SDP packets are required for any YUV format (YUV420, YUV422, >> YUV444). >> + */ >> + if (ctl->ops.update_pending_flush_periph && >> phys_enc->hw_intf->cap->type == INTF_DP && >> + phys_enc->hw_cdm) >> ctl->ops.update_pending_flush_periph(ctl, >> phys_enc->hw_intf->idx); > > Should there be a flush if we are switching from YUV 420 to RGB mode? We only need to flush for the sdp packet, but for msa we do not need to flush. > > Also, I'd say, we should move update_pending_flush_periph invocation > to this patch. Ack > >> skip_flush: >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c >> b/drivers/gpu/drm/msm/dp/dp_display.c >> index 6d764f5b08727..4329435518351 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_display.c >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >> @@ -1399,6 +1399,18 @@ void __exit msm_dp_unregister(void) >> platform_driver_unregister(&dp_display_driver); >> } >> +bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display, >> + const struct drm_display_mode *mode) >> +{ >> + struct dp_display_private *dp; >> + const struct drm_display_info *info; >> + >> + dp = container_of(dp_display, struct dp_display_private, >> dp_display); >> + info = &dp_display->connector->display_info; >> + >> + return dp_panel_vsc_sdp_supported(dp->panel) && >> drm_mode_is_420_only(info, mode); > > YUV 420 modes should be filtered out in mode_valid if VSC SDP is not > supported. Ack. Will change > >> +} >> + >> bool msm_dp_wide_bus_available(const struct msm_dp *dp_display) >> { >> struct dp_display_private *dp; >> diff --git a/drivers/gpu/drm/msm/msm_drv.h >> b/drivers/gpu/drm/msm/msm_drv.h >> index 16a7cbc0b7dd8..b9581bd934e9e 100644 >> --- a/drivers/gpu/drm/msm/msm_drv.h >> +++ b/drivers/gpu/drm/msm/msm_drv.h >> @@ -387,7 +387,8 @@ void __exit msm_dp_unregister(void); >> int msm_dp_modeset_init(struct msm_dp *dp_display, struct >> drm_device *dev, >> struct drm_encoder *encoder); >> void msm_dp_snapshot(struct msm_disp_state *disp_state, struct >> msm_dp *dp_display); >> - >> +bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display, >> + const struct drm_display_mode *mode); >> bool msm_dp_wide_bus_available(const struct msm_dp *dp_display); >> #else >> @@ -409,6 +410,12 @@ static inline void msm_dp_snapshot(struct >> msm_disp_state *disp_state, struct msm >> { >> } >> +static inline bool msm_dp_is_yuv_420_enabled(const struct msm_dp >> *dp_display, >> + const struct drm_display_mode *mode) >> +{ >> + return false; >> +} >> + >> static inline bool msm_dp_wide_bus_available(const struct msm_dp >> *dp_display) >> { >> return false; >
On Sun, 28 Jan 2024 at 07:48, Paloma Arellano <quic_parellan@quicinc.com> wrote: > > > On 1/25/2024 1:57 PM, Dmitry Baryshkov wrote: > > On 25/01/2024 21:38, Paloma Arellano wrote: > >> Adjust the encoder format programming in the case of video mode for DP > >> to accommodate CDM related changes. > >> > >> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> > >> --- > >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 16 +++++++++ > >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 +++++ > >> .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 35 ++++++++++++++++--- > >> drivers/gpu/drm/msm/dp/dp_display.c | 12 +++++++ > >> drivers/gpu/drm/msm/msm_drv.h | 9 ++++- > >> 5 files changed, 75 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 b0896814c1562..99ec53446ad21 100644 > >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >> @@ -222,6 +222,22 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = { > >> 15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10 > >> }; > >> +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, > >> const struct drm_display_mode *mode) > >> +{ > >> + const struct dpu_encoder_virt *dpu_enc; > >> + const struct msm_display_info *disp_info; > >> + struct msm_drm_private *priv; > >> + > >> + dpu_enc = to_dpu_encoder_virt(drm_enc); > >> + disp_info = &dpu_enc->disp_info; > >> + priv = drm_enc->dev->dev_private; > >> + > >> + if (disp_info->intf_type == INTF_DP && > >> + msm_dp_is_yuv_420_enabled(priv->dp[disp_info->h_tile_instance[0]], > >> mode)) > > > > This should not require interacting with DP. If we got here, we must > > be sure that 4:2:0 is supported and can be configured. > Ack. Will drop this function and only check for if the mode is YUV420. > > > >> + return DRM_FORMAT_YUV420; > >> + > >> + return DRM_FORMAT_RGB888; > >> +} > >> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder > >> *drm_enc) > >> { > >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >> index 7b4afa71f1f96..62255d0aa4487 100644 > >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >> @@ -162,6 +162,14 @@ int dpu_encoder_get_vsync_count(struct > >> drm_encoder *drm_enc); > >> */ > >> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder > >> *drm_enc); > >> +/** > >> + * dpu_encoder_get_drm_fmt - return DRM fourcc format > >> + * @drm_enc: Pointer to previously created drm encoder structure > >> + * @mode: Corresponding drm_display_mode for dpu encoder > >> + */ > >> +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, > >> + const struct drm_display_mode *mode); > >> + > >> /** > >> * dpu_encoder_get_crc_values_cnt - get number of physical encoders > >> contained > >> * in virtual encoder that can collect CRC values > >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > >> index e284bf448bdda..a1dde0ff35dc8 100644 > >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > >> @@ -234,6 +234,7 @@ static void > >> dpu_encoder_phys_vid_setup_timing_engine( > >> { > >> struct drm_display_mode mode; > >> struct dpu_hw_intf_timing_params timing_params = { 0 }; > >> + struct dpu_hw_cdm *hw_cdm; > >> const struct dpu_format *fmt = NULL; > >> u32 fmt_fourcc = DRM_FORMAT_RGB888; > >> unsigned long lock_flags; > >> @@ -254,17 +255,26 @@ static void > >> dpu_encoder_phys_vid_setup_timing_engine( > >> DPU_DEBUG_VIDENC(phys_enc, "enabling mode:\n"); > >> drm_mode_debug_printmodeline(&mode); > >> - if (phys_enc->split_role != ENC_ROLE_SOLO) { > >> + hw_cdm = phys_enc->hw_cdm; > >> + if (hw_cdm) { > >> + intf_cfg.cdm = hw_cdm->idx; > >> + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, &mode); > >> + } > >> + > >> + if (phys_enc->split_role != ENC_ROLE_SOLO || > >> + dpu_encoder_get_drm_fmt(phys_enc->parent, &mode) == > >> DRM_FORMAT_YUV420) { > >> mode.hdisplay >>= 1; > >> mode.htotal >>= 1; > >> mode.hsync_start >>= 1; > >> mode.hsync_end >>= 1; > >> + mode.hskew >>= 1; > > > > Separate patch. > Ack. > > > >> DPU_DEBUG_VIDENC(phys_enc, > >> - "split_role %d, halve horizontal %d %d %d %d\n", > >> + "split_role %d, halve horizontal %d %d %d %d %d\n", > >> phys_enc->split_role, > >> mode.hdisplay, mode.htotal, > >> - mode.hsync_start, mode.hsync_end); > >> + mode.hsync_start, mode.hsync_end, > >> + mode.hskew); > >> } > >> drm_mode_to_intf_timing_params(phys_enc, &mode, &timing_params); > >> @@ -412,8 +422,15 @@ static int dpu_encoder_phys_vid_control_vblank_irq( > >> static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys > >> *phys_enc) > >> { > >> struct dpu_hw_ctl *ctl; > >> + struct dpu_hw_cdm *hw_cdm; > >> + const struct dpu_format *fmt = NULL; > >> + u32 fmt_fourcc = DRM_FORMAT_RGB888; > >> ctl = phys_enc->hw_ctl; > >> + hw_cdm = phys_enc->hw_cdm; > >> + if (hw_cdm) > >> + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, > >> &phys_enc->cached_mode); > >> + fmt = dpu_get_dpu_format(fmt_fourcc); > >> DPU_DEBUG_VIDENC(phys_enc, "\n"); > >> @@ -422,6 +439,8 @@ static void dpu_encoder_phys_vid_enable(struct > >> dpu_encoder_phys *phys_enc) > >> dpu_encoder_helper_split_config(phys_enc, > >> phys_enc->hw_intf->idx); > >> + dpu_encoder_helper_phys_setup_cdm(phys_enc, fmt, > >> CDM_CDWN_OUTPUT_HDMI); > > > > If there is no CDM, why do we need to call this? > Inside of dpu_encoder_helper_phys_setup_cdm(), there's a check to see if > there is a hw_cdm. If there is not, then it immediately exits the function. > > > >> + > >> dpu_encoder_phys_vid_setup_timing_engine(phys_enc); > >> /* > >> @@ -437,7 +456,15 @@ static void dpu_encoder_phys_vid_enable(struct > >> dpu_encoder_phys *phys_enc) > >> if (ctl->ops.update_pending_flush_merge_3d && > >> phys_enc->hw_pp->merge_3d) > >> ctl->ops.update_pending_flush_merge_3d(ctl, > >> phys_enc->hw_pp->merge_3d->idx); > >> - if (ctl->ops.update_pending_flush_periph && > >> phys_enc->hw_intf->cap->type == INTF_DP) > >> + if (ctl->ops.update_pending_flush_cdm && phys_enc->hw_cdm) > >> + ctl->ops.update_pending_flush_cdm(ctl, hw_cdm->idx); > >> + > >> + /* > >> + * Peripheral flush must be updated whenever flushing SDP > >> packets is needed. > >> + * SDP packets are required for any YUV format (YUV420, YUV422, > >> YUV444). > >> + */ > >> + if (ctl->ops.update_pending_flush_periph && > >> phys_enc->hw_intf->cap->type == INTF_DP && > >> + phys_enc->hw_cdm) > >> ctl->ops.update_pending_flush_periph(ctl, > >> phys_enc->hw_intf->idx); > > > > Should there be a flush if we are switching from YUV 420 to RGB mode? > We only need to flush for the sdp packet, but for msa we do not need to > flush. What about having SDP with RGB as colorimetry? In other words, if there is a decision point, this one looks incorrect. > > > > Also, I'd say, we should move update_pending_flush_periph invocation > > to this patch. > Ack > > > >> skip_flush: > >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > >> b/drivers/gpu/drm/msm/dp/dp_display.c > >> index 6d764f5b08727..4329435518351 100644 > >> --- a/drivers/gpu/drm/msm/dp/dp_display.c > >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c > >> @@ -1399,6 +1399,18 @@ void __exit msm_dp_unregister(void) > >> platform_driver_unregister(&dp_display_driver); > >> } > >> +bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display, > >> + const struct drm_display_mode *mode) > >> +{ > >> + struct dp_display_private *dp; > >> + const struct drm_display_info *info; > >> + > >> + dp = container_of(dp_display, struct dp_display_private, > >> dp_display); > >> + info = &dp_display->connector->display_info; > >> + > >> + return dp_panel_vsc_sdp_supported(dp->panel) && > >> drm_mode_is_420_only(info, mode); > > > > YUV 420 modes should be filtered out in mode_valid if VSC SDP is not > > supported. > Ack. Will change > > > >> +} > >> + > >> bool msm_dp_wide_bus_available(const struct msm_dp *dp_display) > >> { > >> struct dp_display_private *dp; > >> diff --git a/drivers/gpu/drm/msm/msm_drv.h > >> b/drivers/gpu/drm/msm/msm_drv.h > >> index 16a7cbc0b7dd8..b9581bd934e9e 100644 > >> --- a/drivers/gpu/drm/msm/msm_drv.h > >> +++ b/drivers/gpu/drm/msm/msm_drv.h > >> @@ -387,7 +387,8 @@ void __exit msm_dp_unregister(void); > >> int msm_dp_modeset_init(struct msm_dp *dp_display, struct > >> drm_device *dev, > >> struct drm_encoder *encoder); > >> void msm_dp_snapshot(struct msm_disp_state *disp_state, struct > >> msm_dp *dp_display); > >> - > >> +bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display, > >> + const struct drm_display_mode *mode); > >> bool msm_dp_wide_bus_available(const struct msm_dp *dp_display); > >> #else > >> @@ -409,6 +410,12 @@ static inline void msm_dp_snapshot(struct > >> msm_disp_state *disp_state, struct msm > >> { > >> } > >> +static inline bool msm_dp_is_yuv_420_enabled(const struct msm_dp > >> *dp_display, > >> + const struct drm_display_mode *mode) > >> +{ > >> + return false; > >> +} > >> + > >> static inline bool msm_dp_wide_bus_available(const struct msm_dp > >> *dp_display) > >> { > >> return false; > > -- With best wishes Dmitry
On 1/27/2024 9:55 PM, Dmitry Baryshkov wrote: > On Sun, 28 Jan 2024 at 07:48, Paloma Arellano <quic_parellan@quicinc.com> wrote: >> >> >> On 1/25/2024 1:57 PM, Dmitry Baryshkov wrote: >>> On 25/01/2024 21:38, Paloma Arellano wrote: >>>> Adjust the encoder format programming in the case of video mode for DP >>>> to accommodate CDM related changes. >>>> >>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> >>>> --- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 16 +++++++++ >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 +++++ >>>> .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 35 ++++++++++++++++--- >>>> drivers/gpu/drm/msm/dp/dp_display.c | 12 +++++++ >>>> drivers/gpu/drm/msm/msm_drv.h | 9 ++++- >>>> 5 files changed, 75 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 b0896814c1562..99ec53446ad21 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>> @@ -222,6 +222,22 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = { >>>> 15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10 >>>> }; >>>> +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, >>>> const struct drm_display_mode *mode) >>>> +{ >>>> + const struct dpu_encoder_virt *dpu_enc; >>>> + const struct msm_display_info *disp_info; >>>> + struct msm_drm_private *priv; >>>> + >>>> + dpu_enc = to_dpu_encoder_virt(drm_enc); >>>> + disp_info = &dpu_enc->disp_info; >>>> + priv = drm_enc->dev->dev_private; >>>> + >>>> + if (disp_info->intf_type == INTF_DP && >>>> + msm_dp_is_yuv_420_enabled(priv->dp[disp_info->h_tile_instance[0]], >>>> mode)) >>> >>> This should not require interacting with DP. If we got here, we must >>> be sure that 4:2:0 is supported and can be configured. >> Ack. Will drop this function and only check for if the mode is YUV420. >>> >>>> + return DRM_FORMAT_YUV420; >>>> + >>>> + return DRM_FORMAT_RGB888; >>>> +} >>>> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder >>>> *drm_enc) >>>> { >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>> index 7b4afa71f1f96..62255d0aa4487 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>> @@ -162,6 +162,14 @@ int dpu_encoder_get_vsync_count(struct >>>> drm_encoder *drm_enc); >>>> */ >>>> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder >>>> *drm_enc); >>>> +/** >>>> + * dpu_encoder_get_drm_fmt - return DRM fourcc format >>>> + * @drm_enc: Pointer to previously created drm encoder structure >>>> + * @mode: Corresponding drm_display_mode for dpu encoder >>>> + */ >>>> +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, >>>> + const struct drm_display_mode *mode); >>>> + >>>> /** >>>> * dpu_encoder_get_crc_values_cnt - get number of physical encoders >>>> contained >>>> * in virtual encoder that can collect CRC values >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c >>>> index e284bf448bdda..a1dde0ff35dc8 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c >>>> @@ -234,6 +234,7 @@ static void >>>> dpu_encoder_phys_vid_setup_timing_engine( >>>> { >>>> struct drm_display_mode mode; >>>> struct dpu_hw_intf_timing_params timing_params = { 0 }; >>>> + struct dpu_hw_cdm *hw_cdm; >>>> const struct dpu_format *fmt = NULL; >>>> u32 fmt_fourcc = DRM_FORMAT_RGB888; >>>> unsigned long lock_flags; >>>> @@ -254,17 +255,26 @@ static void >>>> dpu_encoder_phys_vid_setup_timing_engine( >>>> DPU_DEBUG_VIDENC(phys_enc, "enabling mode:\n"); >>>> drm_mode_debug_printmodeline(&mode); >>>> - if (phys_enc->split_role != ENC_ROLE_SOLO) { >>>> + hw_cdm = phys_enc->hw_cdm; >>>> + if (hw_cdm) { >>>> + intf_cfg.cdm = hw_cdm->idx; >>>> + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, &mode); >>>> + } >>>> + >>>> + if (phys_enc->split_role != ENC_ROLE_SOLO || >>>> + dpu_encoder_get_drm_fmt(phys_enc->parent, &mode) == >>>> DRM_FORMAT_YUV420) { >>>> mode.hdisplay >>= 1; >>>> mode.htotal >>= 1; >>>> mode.hsync_start >>= 1; >>>> mode.hsync_end >>= 1; >>>> + mode.hskew >>= 1; >>> >>> Separate patch. >> Ack. >>> >>>> DPU_DEBUG_VIDENC(phys_enc, >>>> - "split_role %d, halve horizontal %d %d %d %d\n", >>>> + "split_role %d, halve horizontal %d %d %d %d %d\n", >>>> phys_enc->split_role, >>>> mode.hdisplay, mode.htotal, >>>> - mode.hsync_start, mode.hsync_end); >>>> + mode.hsync_start, mode.hsync_end, >>>> + mode.hskew); >>>> } >>>> drm_mode_to_intf_timing_params(phys_enc, &mode, &timing_params); >>>> @@ -412,8 +422,15 @@ static int dpu_encoder_phys_vid_control_vblank_irq( >>>> static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys >>>> *phys_enc) >>>> { >>>> struct dpu_hw_ctl *ctl; >>>> + struct dpu_hw_cdm *hw_cdm; >>>> + const struct dpu_format *fmt = NULL; >>>> + u32 fmt_fourcc = DRM_FORMAT_RGB888; >>>> ctl = phys_enc->hw_ctl; >>>> + hw_cdm = phys_enc->hw_cdm; >>>> + if (hw_cdm) >>>> + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, >>>> &phys_enc->cached_mode); >>>> + fmt = dpu_get_dpu_format(fmt_fourcc); >>>> DPU_DEBUG_VIDENC(phys_enc, "\n"); >>>> @@ -422,6 +439,8 @@ static void dpu_encoder_phys_vid_enable(struct >>>> dpu_encoder_phys *phys_enc) >>>> dpu_encoder_helper_split_config(phys_enc, >>>> phys_enc->hw_intf->idx); >>>> + dpu_encoder_helper_phys_setup_cdm(phys_enc, fmt, >>>> CDM_CDWN_OUTPUT_HDMI); >>> >>> If there is no CDM, why do we need to call this? >> Inside of dpu_encoder_helper_phys_setup_cdm(), there's a check to see if >> there is a hw_cdm. If there is not, then it immediately exits the function. >>> >>>> + >>>> dpu_encoder_phys_vid_setup_timing_engine(phys_enc); >>>> /* >>>> @@ -437,7 +456,15 @@ static void dpu_encoder_phys_vid_enable(struct >>>> dpu_encoder_phys *phys_enc) >>>> if (ctl->ops.update_pending_flush_merge_3d && >>>> phys_enc->hw_pp->merge_3d) >>>> ctl->ops.update_pending_flush_merge_3d(ctl, >>>> phys_enc->hw_pp->merge_3d->idx); >>>> - if (ctl->ops.update_pending_flush_periph && >>>> phys_enc->hw_intf->cap->type == INTF_DP) >>>> + if (ctl->ops.update_pending_flush_cdm && phys_enc->hw_cdm) >>>> + ctl->ops.update_pending_flush_cdm(ctl, hw_cdm->idx); >>>> + >>>> + /* >>>> + * Peripheral flush must be updated whenever flushing SDP >>>> packets is needed. >>>> + * SDP packets are required for any YUV format (YUV420, YUV422, >>>> YUV444). >>>> + */ >>>> + if (ctl->ops.update_pending_flush_periph && >>>> phys_enc->hw_intf->cap->type == INTF_DP && >>>> + phys_enc->hw_cdm) >>>> ctl->ops.update_pending_flush_periph(ctl, >>>> phys_enc->hw_intf->idx); >>> >>> Should there be a flush if we are switching from YUV 420 to RGB mode? >> We only need to flush for the sdp packet, but for msa we do not need to >> flush. > > What about having SDP with RGB as colorimetry? In other words, if > there is a decision point, this one looks incorrect. > There are two ways to do it: 1) Use SDP for both RGB and YUV as that supports both. If we implement this policy, then what you are asking for is correct that we will need SDP even to switch back to RGB. But to implement this we will also need to have some sort of state management in the encoder layer about what is the current encoder fmt Vs what is the prev fmt and then trigger peripheral flush only during transitions from RGB to YUV and vice-versa 2) Use SDP only for YUV because MSA does not support YUV formats and use MSA for RGB We decided to implement (2) and there is no significant impact of switching between MSA and SDPs but state management becomes easier. >>> >>> Also, I'd say, we should move update_pending_flush_periph invocation >>> to this patch. >> Ack >>> >>>> skip_flush: >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c >>>> b/drivers/gpu/drm/msm/dp/dp_display.c >>>> index 6d764f5b08727..4329435518351 100644 >>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c >>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >>>> @@ -1399,6 +1399,18 @@ void __exit msm_dp_unregister(void) >>>> platform_driver_unregister(&dp_display_driver); >>>> } >>>> +bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display, >>>> + const struct drm_display_mode *mode) >>>> +{ >>>> + struct dp_display_private *dp; >>>> + const struct drm_display_info *info; >>>> + >>>> + dp = container_of(dp_display, struct dp_display_private, >>>> dp_display); >>>> + info = &dp_display->connector->display_info; >>>> + >>>> + return dp_panel_vsc_sdp_supported(dp->panel) && >>>> drm_mode_is_420_only(info, mode); >>> >>> YUV 420 modes should be filtered out in mode_valid if VSC SDP is not >>> supported. >> Ack. Will change >>> >>>> +} >>>> + >>>> bool msm_dp_wide_bus_available(const struct msm_dp *dp_display) >>>> { >>>> struct dp_display_private *dp; >>>> diff --git a/drivers/gpu/drm/msm/msm_drv.h >>>> b/drivers/gpu/drm/msm/msm_drv.h >>>> index 16a7cbc0b7dd8..b9581bd934e9e 100644 >>>> --- a/drivers/gpu/drm/msm/msm_drv.h >>>> +++ b/drivers/gpu/drm/msm/msm_drv.h >>>> @@ -387,7 +387,8 @@ void __exit msm_dp_unregister(void); >>>> int msm_dp_modeset_init(struct msm_dp *dp_display, struct >>>> drm_device *dev, >>>> struct drm_encoder *encoder); >>>> void msm_dp_snapshot(struct msm_disp_state *disp_state, struct >>>> msm_dp *dp_display); >>>> - >>>> +bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display, >>>> + const struct drm_display_mode *mode); >>>> bool msm_dp_wide_bus_available(const struct msm_dp *dp_display); >>>> #else >>>> @@ -409,6 +410,12 @@ static inline void msm_dp_snapshot(struct >>>> msm_disp_state *disp_state, struct msm >>>> { >>>> } >>>> +static inline bool msm_dp_is_yuv_420_enabled(const struct msm_dp >>>> *dp_display, >>>> + const struct drm_display_mode *mode) >>>> +{ >>>> + return false; >>>> +} >>>> + >>>> static inline bool msm_dp_wide_bus_available(const struct msm_dp >>>> *dp_display) >>>> { >>>> return false; >>> > > > > -- > With best wishes > Dmitry
On Mon, 29 Jan 2024 at 04:58, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 1/27/2024 9:55 PM, Dmitry Baryshkov wrote: > > On Sun, 28 Jan 2024 at 07:48, Paloma Arellano <quic_parellan@quicinc.com> wrote: > >> > >> > >> On 1/25/2024 1:57 PM, Dmitry Baryshkov wrote: > >>> On 25/01/2024 21:38, Paloma Arellano wrote: > >>>> Adjust the encoder format programming in the case of video mode for DP > >>>> to accommodate CDM related changes. > >>>> > >>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> > >>>> --- > >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 16 +++++++++ > >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 +++++ > >>>> .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 35 ++++++++++++++++--- > >>>> drivers/gpu/drm/msm/dp/dp_display.c | 12 +++++++ > >>>> drivers/gpu/drm/msm/msm_drv.h | 9 ++++- > >>>> 5 files changed, 75 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 b0896814c1562..99ec53446ad21 100644 > >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >>>> @@ -222,6 +222,22 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = { > >>>> 15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10 > >>>> }; > >>>> +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, > >>>> const struct drm_display_mode *mode) > >>>> +{ > >>>> + const struct dpu_encoder_virt *dpu_enc; > >>>> + const struct msm_display_info *disp_info; > >>>> + struct msm_drm_private *priv; > >>>> + > >>>> + dpu_enc = to_dpu_encoder_virt(drm_enc); > >>>> + disp_info = &dpu_enc->disp_info; > >>>> + priv = drm_enc->dev->dev_private; > >>>> + > >>>> + if (disp_info->intf_type == INTF_DP && > >>>> + msm_dp_is_yuv_420_enabled(priv->dp[disp_info->h_tile_instance[0]], > >>>> mode)) > >>> > >>> This should not require interacting with DP. If we got here, we must > >>> be sure that 4:2:0 is supported and can be configured. > >> Ack. Will drop this function and only check for if the mode is YUV420. > >>> > >>>> + return DRM_FORMAT_YUV420; > >>>> + > >>>> + return DRM_FORMAT_RGB888; > >>>> +} > >>>> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder > >>>> *drm_enc) > >>>> { > >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >>>> index 7b4afa71f1f96..62255d0aa4487 100644 > >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >>>> @@ -162,6 +162,14 @@ int dpu_encoder_get_vsync_count(struct > >>>> drm_encoder *drm_enc); > >>>> */ > >>>> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder > >>>> *drm_enc); > >>>> +/** > >>>> + * dpu_encoder_get_drm_fmt - return DRM fourcc format > >>>> + * @drm_enc: Pointer to previously created drm encoder structure > >>>> + * @mode: Corresponding drm_display_mode for dpu encoder > >>>> + */ > >>>> +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, > >>>> + const struct drm_display_mode *mode); > >>>> + > >>>> /** > >>>> * dpu_encoder_get_crc_values_cnt - get number of physical encoders > >>>> contained > >>>> * in virtual encoder that can collect CRC values > >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > >>>> index e284bf448bdda..a1dde0ff35dc8 100644 > >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > >>>> @@ -234,6 +234,7 @@ static void > >>>> dpu_encoder_phys_vid_setup_timing_engine( > >>>> { > >>>> struct drm_display_mode mode; > >>>> struct dpu_hw_intf_timing_params timing_params = { 0 }; > >>>> + struct dpu_hw_cdm *hw_cdm; > >>>> const struct dpu_format *fmt = NULL; > >>>> u32 fmt_fourcc = DRM_FORMAT_RGB888; > >>>> unsigned long lock_flags; > >>>> @@ -254,17 +255,26 @@ static void > >>>> dpu_encoder_phys_vid_setup_timing_engine( > >>>> DPU_DEBUG_VIDENC(phys_enc, "enabling mode:\n"); > >>>> drm_mode_debug_printmodeline(&mode); > >>>> - if (phys_enc->split_role != ENC_ROLE_SOLO) { > >>>> + hw_cdm = phys_enc->hw_cdm; > >>>> + if (hw_cdm) { > >>>> + intf_cfg.cdm = hw_cdm->idx; > >>>> + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, &mode); > >>>> + } > >>>> + > >>>> + if (phys_enc->split_role != ENC_ROLE_SOLO || > >>>> + dpu_encoder_get_drm_fmt(phys_enc->parent, &mode) == > >>>> DRM_FORMAT_YUV420) { > >>>> mode.hdisplay >>= 1; > >>>> mode.htotal >>= 1; > >>>> mode.hsync_start >>= 1; > >>>> mode.hsync_end >>= 1; > >>>> + mode.hskew >>= 1; > >>> > >>> Separate patch. > >> Ack. > >>> > >>>> DPU_DEBUG_VIDENC(phys_enc, > >>>> - "split_role %d, halve horizontal %d %d %d %d\n", > >>>> + "split_role %d, halve horizontal %d %d %d %d %d\n", > >>>> phys_enc->split_role, > >>>> mode.hdisplay, mode.htotal, > >>>> - mode.hsync_start, mode.hsync_end); > >>>> + mode.hsync_start, mode.hsync_end, > >>>> + mode.hskew); > >>>> } > >>>> drm_mode_to_intf_timing_params(phys_enc, &mode, &timing_params); > >>>> @@ -412,8 +422,15 @@ static int dpu_encoder_phys_vid_control_vblank_irq( > >>>> static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys > >>>> *phys_enc) > >>>> { > >>>> struct dpu_hw_ctl *ctl; > >>>> + struct dpu_hw_cdm *hw_cdm; > >>>> + const struct dpu_format *fmt = NULL; > >>>> + u32 fmt_fourcc = DRM_FORMAT_RGB888; > >>>> ctl = phys_enc->hw_ctl; > >>>> + hw_cdm = phys_enc->hw_cdm; > >>>> + if (hw_cdm) > >>>> + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, > >>>> &phys_enc->cached_mode); > >>>> + fmt = dpu_get_dpu_format(fmt_fourcc); > >>>> DPU_DEBUG_VIDENC(phys_enc, "\n"); > >>>> @@ -422,6 +439,8 @@ static void dpu_encoder_phys_vid_enable(struct > >>>> dpu_encoder_phys *phys_enc) > >>>> dpu_encoder_helper_split_config(phys_enc, > >>>> phys_enc->hw_intf->idx); > >>>> + dpu_encoder_helper_phys_setup_cdm(phys_enc, fmt, > >>>> CDM_CDWN_OUTPUT_HDMI); > >>> > >>> If there is no CDM, why do we need to call this? > >> Inside of dpu_encoder_helper_phys_setup_cdm(), there's a check to see if > >> there is a hw_cdm. If there is not, then it immediately exits the function. > >>> > >>>> + > >>>> dpu_encoder_phys_vid_setup_timing_engine(phys_enc); > >>>> /* > >>>> @@ -437,7 +456,15 @@ static void dpu_encoder_phys_vid_enable(struct > >>>> dpu_encoder_phys *phys_enc) > >>>> if (ctl->ops.update_pending_flush_merge_3d && > >>>> phys_enc->hw_pp->merge_3d) > >>>> ctl->ops.update_pending_flush_merge_3d(ctl, > >>>> phys_enc->hw_pp->merge_3d->idx); > >>>> - if (ctl->ops.update_pending_flush_periph && > >>>> phys_enc->hw_intf->cap->type == INTF_DP) > >>>> + if (ctl->ops.update_pending_flush_cdm && phys_enc->hw_cdm) > >>>> + ctl->ops.update_pending_flush_cdm(ctl, hw_cdm->idx); > >>>> + > >>>> + /* > >>>> + * Peripheral flush must be updated whenever flushing SDP > >>>> packets is needed. > >>>> + * SDP packets are required for any YUV format (YUV420, YUV422, > >>>> YUV444). > >>>> + */ > >>>> + if (ctl->ops.update_pending_flush_periph && > >>>> phys_enc->hw_intf->cap->type == INTF_DP && > >>>> + phys_enc->hw_cdm) > >>>> ctl->ops.update_pending_flush_periph(ctl, > >>>> phys_enc->hw_intf->idx); > >>> > >>> Should there be a flush if we are switching from YUV 420 to RGB mode? > >> We only need to flush for the sdp packet, but for msa we do not need to > >> flush. > > > > What about having SDP with RGB as colorimetry? In other words, if > > there is a decision point, this one looks incorrect. > > > > There are two ways to do it: > > 1) Use SDP for both RGB and YUV as that supports both. If we implement > this policy, then what you are asking for is correct that we will need > SDP even to switch back to RGB. But to implement this we will also need > to have some sort of state management in the encoder layer about what is > the current encoder fmt Vs what is the prev fmt and then trigger > peripheral flush only during transitions from RGB to YUV and vice-versa > > 2) Use SDP only for YUV because MSA does not support YUV formats and use > MSA for RGB > > We decided to implement (2) and there is no significant impact of > switching between MSA and SDPs but state management becomes easier. Yes. However as you wrote, there might be other usecases concerning SDP. Having this in mind, it sounds like the driver should decide whether to flush peripheral at a different place (when the SDP infoframe is being updated?). And the dpu_encoder_phys_vid_enable() should use this previous decision. Maybe this should be a part of msm_dp_ API, something like msm_dp_needs_peripheral_flush()? > > >>> > >>> Also, I'd say, we should move update_pending_flush_periph invocation > >>> to this patch. > >> Ack > >>> > >>>> skip_flush: > >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > >>>> b/drivers/gpu/drm/msm/dp/dp_display.c > >>>> index 6d764f5b08727..4329435518351 100644 > >>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c > >>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c > >>>> @@ -1399,6 +1399,18 @@ void __exit msm_dp_unregister(void) > >>>> platform_driver_unregister(&dp_display_driver); > >>>> } > >>>> +bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display, > >>>> + const struct drm_display_mode *mode) > >>>> +{ > >>>> + struct dp_display_private *dp; > >>>> + const struct drm_display_info *info; > >>>> + > >>>> + dp = container_of(dp_display, struct dp_display_private, > >>>> dp_display); > >>>> + info = &dp_display->connector->display_info; > >>>> + > >>>> + return dp_panel_vsc_sdp_supported(dp->panel) && > >>>> drm_mode_is_420_only(info, mode); > >>> > >>> YUV 420 modes should be filtered out in mode_valid if VSC SDP is not > >>> supported. > >> Ack. Will change > >>> > >>>> +} > >>>> + > >>>> bool msm_dp_wide_bus_available(const struct msm_dp *dp_display) > >>>> { > >>>> struct dp_display_private *dp; > >>>> diff --git a/drivers/gpu/drm/msm/msm_drv.h > >>>> b/drivers/gpu/drm/msm/msm_drv.h > >>>> index 16a7cbc0b7dd8..b9581bd934e9e 100644 > >>>> --- a/drivers/gpu/drm/msm/msm_drv.h > >>>> +++ b/drivers/gpu/drm/msm/msm_drv.h > >>>> @@ -387,7 +387,8 @@ void __exit msm_dp_unregister(void); > >>>> int msm_dp_modeset_init(struct msm_dp *dp_display, struct > >>>> drm_device *dev, > >>>> struct drm_encoder *encoder); > >>>> void msm_dp_snapshot(struct msm_disp_state *disp_state, struct > >>>> msm_dp *dp_display); > >>>> - > >>>> +bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display, > >>>> + const struct drm_display_mode *mode); > >>>> bool msm_dp_wide_bus_available(const struct msm_dp *dp_display); > >>>> #else > >>>> @@ -409,6 +410,12 @@ static inline void msm_dp_snapshot(struct > >>>> msm_disp_state *disp_state, struct msm > >>>> { > >>>> } > >>>> +static inline bool msm_dp_is_yuv_420_enabled(const struct msm_dp > >>>> *dp_display, > >>>> + const struct drm_display_mode *mode) > >>>> +{ > >>>> + return false; > >>>> +} > >>>> + > >>>> static inline bool msm_dp_wide_bus_available(const struct msm_dp > >>>> *dp_display) > >>>> { > >>>> return false; > >>> > > > > > > > > -- > > With best wishes > > Dmitry
On 1/28/2024 7:42 PM, Dmitry Baryshkov wrote: > On Mon, 29 Jan 2024 at 04:58, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 1/27/2024 9:55 PM, Dmitry Baryshkov wrote: >>> On Sun, 28 Jan 2024 at 07:48, Paloma Arellano <quic_parellan@quicinc.com> wrote: >>>> >>>> >>>> On 1/25/2024 1:57 PM, Dmitry Baryshkov wrote: >>>>> On 25/01/2024 21:38, Paloma Arellano wrote: >>>>>> Adjust the encoder format programming in the case of video mode for DP >>>>>> to accommodate CDM related changes. >>>>>> >>>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> >>>>>> --- >>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 16 +++++++++ >>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 +++++ >>>>>> .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 35 ++++++++++++++++--- >>>>>> drivers/gpu/drm/msm/dp/dp_display.c | 12 +++++++ >>>>>> drivers/gpu/drm/msm/msm_drv.h | 9 ++++- >>>>>> 5 files changed, 75 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 b0896814c1562..99ec53446ad21 100644 >>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>>>> @@ -222,6 +222,22 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = { >>>>>> 15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10 >>>>>> }; >>>>>> +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, >>>>>> const struct drm_display_mode *mode) >>>>>> +{ >>>>>> + const struct dpu_encoder_virt *dpu_enc; >>>>>> + const struct msm_display_info *disp_info; >>>>>> + struct msm_drm_private *priv; >>>>>> + >>>>>> + dpu_enc = to_dpu_encoder_virt(drm_enc); >>>>>> + disp_info = &dpu_enc->disp_info; >>>>>> + priv = drm_enc->dev->dev_private; >>>>>> + >>>>>> + if (disp_info->intf_type == INTF_DP && >>>>>> + msm_dp_is_yuv_420_enabled(priv->dp[disp_info->h_tile_instance[0]], >>>>>> mode)) >>>>> >>>>> This should not require interacting with DP. If we got here, we must >>>>> be sure that 4:2:0 is supported and can be configured. >>>> Ack. Will drop this function and only check for if the mode is YUV420. >>>>> >>>>>> + return DRM_FORMAT_YUV420; >>>>>> + >>>>>> + return DRM_FORMAT_RGB888; >>>>>> +} >>>>>> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder >>>>>> *drm_enc) >>>>>> { >>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>>>> index 7b4afa71f1f96..62255d0aa4487 100644 >>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>>>> @@ -162,6 +162,14 @@ int dpu_encoder_get_vsync_count(struct >>>>>> drm_encoder *drm_enc); >>>>>> */ >>>>>> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder >>>>>> *drm_enc); >>>>>> +/** >>>>>> + * dpu_encoder_get_drm_fmt - return DRM fourcc format >>>>>> + * @drm_enc: Pointer to previously created drm encoder structure >>>>>> + * @mode: Corresponding drm_display_mode for dpu encoder >>>>>> + */ >>>>>> +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, >>>>>> + const struct drm_display_mode *mode); >>>>>> + >>>>>> /** >>>>>> * dpu_encoder_get_crc_values_cnt - get number of physical encoders >>>>>> contained >>>>>> * in virtual encoder that can collect CRC values >>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c >>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c >>>>>> index e284bf448bdda..a1dde0ff35dc8 100644 >>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c >>>>>> @@ -234,6 +234,7 @@ static void >>>>>> dpu_encoder_phys_vid_setup_timing_engine( >>>>>> { >>>>>> struct drm_display_mode mode; >>>>>> struct dpu_hw_intf_timing_params timing_params = { 0 }; >>>>>> + struct dpu_hw_cdm *hw_cdm; >>>>>> const struct dpu_format *fmt = NULL; >>>>>> u32 fmt_fourcc = DRM_FORMAT_RGB888; >>>>>> unsigned long lock_flags; >>>>>> @@ -254,17 +255,26 @@ static void >>>>>> dpu_encoder_phys_vid_setup_timing_engine( >>>>>> DPU_DEBUG_VIDENC(phys_enc, "enabling mode:\n"); >>>>>> drm_mode_debug_printmodeline(&mode); >>>>>> - if (phys_enc->split_role != ENC_ROLE_SOLO) { >>>>>> + hw_cdm = phys_enc->hw_cdm; >>>>>> + if (hw_cdm) { >>>>>> + intf_cfg.cdm = hw_cdm->idx; >>>>>> + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, &mode); >>>>>> + } >>>>>> + >>>>>> + if (phys_enc->split_role != ENC_ROLE_SOLO || >>>>>> + dpu_encoder_get_drm_fmt(phys_enc->parent, &mode) == >>>>>> DRM_FORMAT_YUV420) { >>>>>> mode.hdisplay >>= 1; >>>>>> mode.htotal >>= 1; >>>>>> mode.hsync_start >>= 1; >>>>>> mode.hsync_end >>= 1; >>>>>> + mode.hskew >>= 1; >>>>> >>>>> Separate patch. >>>> Ack. >>>>> >>>>>> DPU_DEBUG_VIDENC(phys_enc, >>>>>> - "split_role %d, halve horizontal %d %d %d %d\n", >>>>>> + "split_role %d, halve horizontal %d %d %d %d %d\n", >>>>>> phys_enc->split_role, >>>>>> mode.hdisplay, mode.htotal, >>>>>> - mode.hsync_start, mode.hsync_end); >>>>>> + mode.hsync_start, mode.hsync_end, >>>>>> + mode.hskew); >>>>>> } >>>>>> drm_mode_to_intf_timing_params(phys_enc, &mode, &timing_params); >>>>>> @@ -412,8 +422,15 @@ static int dpu_encoder_phys_vid_control_vblank_irq( >>>>>> static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys >>>>>> *phys_enc) >>>>>> { >>>>>> struct dpu_hw_ctl *ctl; >>>>>> + struct dpu_hw_cdm *hw_cdm; >>>>>> + const struct dpu_format *fmt = NULL; >>>>>> + u32 fmt_fourcc = DRM_FORMAT_RGB888; >>>>>> ctl = phys_enc->hw_ctl; >>>>>> + hw_cdm = phys_enc->hw_cdm; >>>>>> + if (hw_cdm) >>>>>> + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, >>>>>> &phys_enc->cached_mode); >>>>>> + fmt = dpu_get_dpu_format(fmt_fourcc); >>>>>> DPU_DEBUG_VIDENC(phys_enc, "\n"); >>>>>> @@ -422,6 +439,8 @@ static void dpu_encoder_phys_vid_enable(struct >>>>>> dpu_encoder_phys *phys_enc) >>>>>> dpu_encoder_helper_split_config(phys_enc, >>>>>> phys_enc->hw_intf->idx); >>>>>> + dpu_encoder_helper_phys_setup_cdm(phys_enc, fmt, >>>>>> CDM_CDWN_OUTPUT_HDMI); >>>>> >>>>> If there is no CDM, why do we need to call this? >>>> Inside of dpu_encoder_helper_phys_setup_cdm(), there's a check to see if >>>> there is a hw_cdm. If there is not, then it immediately exits the function. >>>>> >>>>>> + >>>>>> dpu_encoder_phys_vid_setup_timing_engine(phys_enc); >>>>>> /* >>>>>> @@ -437,7 +456,15 @@ static void dpu_encoder_phys_vid_enable(struct >>>>>> dpu_encoder_phys *phys_enc) >>>>>> if (ctl->ops.update_pending_flush_merge_3d && >>>>>> phys_enc->hw_pp->merge_3d) >>>>>> ctl->ops.update_pending_flush_merge_3d(ctl, >>>>>> phys_enc->hw_pp->merge_3d->idx); >>>>>> - if (ctl->ops.update_pending_flush_periph && >>>>>> phys_enc->hw_intf->cap->type == INTF_DP) >>>>>> + if (ctl->ops.update_pending_flush_cdm && phys_enc->hw_cdm) >>>>>> + ctl->ops.update_pending_flush_cdm(ctl, hw_cdm->idx); >>>>>> + >>>>>> + /* >>>>>> + * Peripheral flush must be updated whenever flushing SDP >>>>>> packets is needed. >>>>>> + * SDP packets are required for any YUV format (YUV420, YUV422, >>>>>> YUV444). >>>>>> + */ >>>>>> + if (ctl->ops.update_pending_flush_periph && >>>>>> phys_enc->hw_intf->cap->type == INTF_DP && >>>>>> + phys_enc->hw_cdm) >>>>>> ctl->ops.update_pending_flush_periph(ctl, >>>>>> phys_enc->hw_intf->idx); >>>>> >>>>> Should there be a flush if we are switching from YUV 420 to RGB mode? >>>> We only need to flush for the sdp packet, but for msa we do not need to >>>> flush. >>> >>> What about having SDP with RGB as colorimetry? In other words, if >>> there is a decision point, this one looks incorrect. >>> >> >> There are two ways to do it: >> >> 1) Use SDP for both RGB and YUV as that supports both. If we implement >> this policy, then what you are asking for is correct that we will need >> SDP even to switch back to RGB. But to implement this we will also need >> to have some sort of state management in the encoder layer about what is >> the current encoder fmt Vs what is the prev fmt and then trigger >> peripheral flush only during transitions from RGB to YUV and vice-versa >> >> 2) Use SDP only for YUV because MSA does not support YUV formats and use >> MSA for RGB >> >> We decided to implement (2) and there is no significant impact of >> switching between MSA and SDPs but state management becomes easier. > > Yes. However as you wrote, there might be other usecases concerning > SDP. Having this in mind, it sounds like the driver should decide > whether to flush peripheral at a different place (when the SDP > infoframe is being updated?). And the dpu_encoder_phys_vid_enable() > should use this previous decision. Maybe this should be a part of > msm_dp_ API, something like msm_dp_needs_peripheral_flush()? > Correct. The decision to flush peripheral or not certainly comes from the peripheral itself . In this case its DP. I think perhaps the usage of hw_cdm here makes it hard to understand that but the idea behind this was that in the change "drm/msm/dpu: reserve CDM blocks for DP if mode is YUV420 ", hw_cdm is assigned only if msm_dp_is_yuv_420_enabled() returns true. So in some sense, the API you are asking for is already msm_dp_is_yuv_420_enabled() but we can rename that to msm_dp_needs_periph_flush(). The issue here is the phys layer cannot call msm_dp_is_yuv_420_enabled() so its kind of round-about by checking hw_cdm. The check is not entirely wrong because for DP, we need hw_cdm for all YUV formats and not just 420 but the relationship is maybe not clear but I thought the comment above that would help a bit: /* * Peripheral flush must be updated whenever flushing SDP packets is needed. * SDP packets are required for any YUV format (YUV420, YUV422, YUV444). */ The only other way I can think of is maybe we need to introduce a new phys variable called phys_enc->needs_update_periph_flush and we can set that in dpu_encoder.c as that can call msm_dp_needs_periph_flush(). What do you think of this way? >> >>>>> >>>>> Also, I'd say, we should move update_pending_flush_periph invocation >>>>> to this patch. >>>> Ack >>>>> >>>>>> skip_flush: >>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c >>>>>> b/drivers/gpu/drm/msm/dp/dp_display.c >>>>>> index 6d764f5b08727..4329435518351 100644 >>>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c >>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >>>>>> @@ -1399,6 +1399,18 @@ void __exit msm_dp_unregister(void) >>>>>> platform_driver_unregister(&dp_display_driver); >>>>>> } >>>>>> +bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display, >>>>>> + const struct drm_display_mode *mode) >>>>>> +{ >>>>>> + struct dp_display_private *dp; >>>>>> + const struct drm_display_info *info; >>>>>> + >>>>>> + dp = container_of(dp_display, struct dp_display_private, >>>>>> dp_display); >>>>>> + info = &dp_display->connector->display_info; >>>>>> + >>>>>> + return dp_panel_vsc_sdp_supported(dp->panel) && >>>>>> drm_mode_is_420_only(info, mode); >>>>> >>>>> YUV 420 modes should be filtered out in mode_valid if VSC SDP is not >>>>> supported. >>>> Ack. Will change >>>>> >>>>>> +} >>>>>> + >>>>>> bool msm_dp_wide_bus_available(const struct msm_dp *dp_display) >>>>>> { >>>>>> struct dp_display_private *dp; >>>>>> diff --git a/drivers/gpu/drm/msm/msm_drv.h >>>>>> b/drivers/gpu/drm/msm/msm_drv.h >>>>>> index 16a7cbc0b7dd8..b9581bd934e9e 100644 >>>>>> --- a/drivers/gpu/drm/msm/msm_drv.h >>>>>> +++ b/drivers/gpu/drm/msm/msm_drv.h >>>>>> @@ -387,7 +387,8 @@ void __exit msm_dp_unregister(void); >>>>>> int msm_dp_modeset_init(struct msm_dp *dp_display, struct >>>>>> drm_device *dev, >>>>>> struct drm_encoder *encoder); >>>>>> void msm_dp_snapshot(struct msm_disp_state *disp_state, struct >>>>>> msm_dp *dp_display); >>>>>> - >>>>>> +bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display, >>>>>> + const struct drm_display_mode *mode); >>>>>> bool msm_dp_wide_bus_available(const struct msm_dp *dp_display); >>>>>> #else >>>>>> @@ -409,6 +410,12 @@ static inline void msm_dp_snapshot(struct >>>>>> msm_disp_state *disp_state, struct msm >>>>>> { >>>>>> } >>>>>> +static inline bool msm_dp_is_yuv_420_enabled(const struct msm_dp >>>>>> *dp_display, >>>>>> + const struct drm_display_mode *mode) >>>>>> +{ >>>>>> + return false; >>>>>> +} >>>>>> + >>>>>> static inline bool msm_dp_wide_bus_available(const struct msm_dp >>>>>> *dp_display) >>>>>> { >>>>>> return false; >>>>> >>> >>> >>> >>> -- >>> With best wishes >>> Dmitry > > >
On Mon, 29 Jan 2024 at 07:03, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 1/28/2024 7:42 PM, Dmitry Baryshkov wrote: > > On Mon, 29 Jan 2024 at 04:58, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >> > >> > >> > >> On 1/27/2024 9:55 PM, Dmitry Baryshkov wrote: > >>> On Sun, 28 Jan 2024 at 07:48, Paloma Arellano <quic_parellan@quicinc.com> wrote: > >>>> > >>>> > >>>> On 1/25/2024 1:57 PM, Dmitry Baryshkov wrote: > >>>>> On 25/01/2024 21:38, Paloma Arellano wrote: > >>>>>> Adjust the encoder format programming in the case of video mode for DP > >>>>>> to accommodate CDM related changes. > >>>>>> > >>>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> > >>>>>> --- > >>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 16 +++++++++ > >>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 +++++ > >>>>>> .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 35 ++++++++++++++++--- > >>>>>> drivers/gpu/drm/msm/dp/dp_display.c | 12 +++++++ > >>>>>> drivers/gpu/drm/msm/msm_drv.h | 9 ++++- > >>>>>> 5 files changed, 75 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 b0896814c1562..99ec53446ad21 100644 > >>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >>>>>> @@ -222,6 +222,22 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = { > >>>>>> 15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10 > >>>>>> }; > >>>>>> +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, > >>>>>> const struct drm_display_mode *mode) > >>>>>> +{ > >>>>>> + const struct dpu_encoder_virt *dpu_enc; > >>>>>> + const struct msm_display_info *disp_info; > >>>>>> + struct msm_drm_private *priv; > >>>>>> + > >>>>>> + dpu_enc = to_dpu_encoder_virt(drm_enc); > >>>>>> + disp_info = &dpu_enc->disp_info; > >>>>>> + priv = drm_enc->dev->dev_private; > >>>>>> + > >>>>>> + if (disp_info->intf_type == INTF_DP && > >>>>>> + msm_dp_is_yuv_420_enabled(priv->dp[disp_info->h_tile_instance[0]], > >>>>>> mode)) > >>>>> > >>>>> This should not require interacting with DP. If we got here, we must > >>>>> be sure that 4:2:0 is supported and can be configured. > >>>> Ack. Will drop this function and only check for if the mode is YUV420. > >>>>> > >>>>>> + return DRM_FORMAT_YUV420; > >>>>>> + > >>>>>> + return DRM_FORMAT_RGB888; > >>>>>> +} > >>>>>> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder > >>>>>> *drm_enc) > >>>>>> { > >>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >>>>>> index 7b4afa71f1f96..62255d0aa4487 100644 > >>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >>>>>> @@ -162,6 +162,14 @@ int dpu_encoder_get_vsync_count(struct > >>>>>> drm_encoder *drm_enc); > >>>>>> */ > >>>>>> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder > >>>>>> *drm_enc); > >>>>>> +/** > >>>>>> + * dpu_encoder_get_drm_fmt - return DRM fourcc format > >>>>>> + * @drm_enc: Pointer to previously created drm encoder structure > >>>>>> + * @mode: Corresponding drm_display_mode for dpu encoder > >>>>>> + */ > >>>>>> +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, > >>>>>> + const struct drm_display_mode *mode); > >>>>>> + > >>>>>> /** > >>>>>> * dpu_encoder_get_crc_values_cnt - get number of physical encoders > >>>>>> contained > >>>>>> * in virtual encoder that can collect CRC values > >>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > >>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > >>>>>> index e284bf448bdda..a1dde0ff35dc8 100644 > >>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > >>>>>> @@ -234,6 +234,7 @@ static void > >>>>>> dpu_encoder_phys_vid_setup_timing_engine( > >>>>>> { > >>>>>> struct drm_display_mode mode; > >>>>>> struct dpu_hw_intf_timing_params timing_params = { 0 }; > >>>>>> + struct dpu_hw_cdm *hw_cdm; > >>>>>> const struct dpu_format *fmt = NULL; > >>>>>> u32 fmt_fourcc = DRM_FORMAT_RGB888; > >>>>>> unsigned long lock_flags; > >>>>>> @@ -254,17 +255,26 @@ static void > >>>>>> dpu_encoder_phys_vid_setup_timing_engine( > >>>>>> DPU_DEBUG_VIDENC(phys_enc, "enabling mode:\n"); > >>>>>> drm_mode_debug_printmodeline(&mode); > >>>>>> - if (phys_enc->split_role != ENC_ROLE_SOLO) { > >>>>>> + hw_cdm = phys_enc->hw_cdm; > >>>>>> + if (hw_cdm) { > >>>>>> + intf_cfg.cdm = hw_cdm->idx; > >>>>>> + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, &mode); > >>>>>> + } > >>>>>> + > >>>>>> + if (phys_enc->split_role != ENC_ROLE_SOLO || > >>>>>> + dpu_encoder_get_drm_fmt(phys_enc->parent, &mode) == > >>>>>> DRM_FORMAT_YUV420) { > >>>>>> mode.hdisplay >>= 1; > >>>>>> mode.htotal >>= 1; > >>>>>> mode.hsync_start >>= 1; > >>>>>> mode.hsync_end >>= 1; > >>>>>> + mode.hskew >>= 1; > >>>>> > >>>>> Separate patch. > >>>> Ack. > >>>>> > >>>>>> DPU_DEBUG_VIDENC(phys_enc, > >>>>>> - "split_role %d, halve horizontal %d %d %d %d\n", > >>>>>> + "split_role %d, halve horizontal %d %d %d %d %d\n", > >>>>>> phys_enc->split_role, > >>>>>> mode.hdisplay, mode.htotal, > >>>>>> - mode.hsync_start, mode.hsync_end); > >>>>>> + mode.hsync_start, mode.hsync_end, > >>>>>> + mode.hskew); > >>>>>> } > >>>>>> drm_mode_to_intf_timing_params(phys_enc, &mode, &timing_params); > >>>>>> @@ -412,8 +422,15 @@ static int dpu_encoder_phys_vid_control_vblank_irq( > >>>>>> static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys > >>>>>> *phys_enc) > >>>>>> { > >>>>>> struct dpu_hw_ctl *ctl; > >>>>>> + struct dpu_hw_cdm *hw_cdm; > >>>>>> + const struct dpu_format *fmt = NULL; > >>>>>> + u32 fmt_fourcc = DRM_FORMAT_RGB888; > >>>>>> ctl = phys_enc->hw_ctl; > >>>>>> + hw_cdm = phys_enc->hw_cdm; > >>>>>> + if (hw_cdm) > >>>>>> + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, > >>>>>> &phys_enc->cached_mode); > >>>>>> + fmt = dpu_get_dpu_format(fmt_fourcc); > >>>>>> DPU_DEBUG_VIDENC(phys_enc, "\n"); > >>>>>> @@ -422,6 +439,8 @@ static void dpu_encoder_phys_vid_enable(struct > >>>>>> dpu_encoder_phys *phys_enc) > >>>>>> dpu_encoder_helper_split_config(phys_enc, > >>>>>> phys_enc->hw_intf->idx); > >>>>>> + dpu_encoder_helper_phys_setup_cdm(phys_enc, fmt, > >>>>>> CDM_CDWN_OUTPUT_HDMI); > >>>>> > >>>>> If there is no CDM, why do we need to call this? > >>>> Inside of dpu_encoder_helper_phys_setup_cdm(), there's a check to see if > >>>> there is a hw_cdm. If there is not, then it immediately exits the function. > >>>>> > >>>>>> + > >>>>>> dpu_encoder_phys_vid_setup_timing_engine(phys_enc); > >>>>>> /* > >>>>>> @@ -437,7 +456,15 @@ static void dpu_encoder_phys_vid_enable(struct > >>>>>> dpu_encoder_phys *phys_enc) > >>>>>> if (ctl->ops.update_pending_flush_merge_3d && > >>>>>> phys_enc->hw_pp->merge_3d) > >>>>>> ctl->ops.update_pending_flush_merge_3d(ctl, > >>>>>> phys_enc->hw_pp->merge_3d->idx); > >>>>>> - if (ctl->ops.update_pending_flush_periph && > >>>>>> phys_enc->hw_intf->cap->type == INTF_DP) > >>>>>> + if (ctl->ops.update_pending_flush_cdm && phys_enc->hw_cdm) > >>>>>> + ctl->ops.update_pending_flush_cdm(ctl, hw_cdm->idx); > >>>>>> + > >>>>>> + /* > >>>>>> + * Peripheral flush must be updated whenever flushing SDP > >>>>>> packets is needed. > >>>>>> + * SDP packets are required for any YUV format (YUV420, YUV422, > >>>>>> YUV444). > >>>>>> + */ > >>>>>> + if (ctl->ops.update_pending_flush_periph && > >>>>>> phys_enc->hw_intf->cap->type == INTF_DP && > >>>>>> + phys_enc->hw_cdm) > >>>>>> ctl->ops.update_pending_flush_periph(ctl, > >>>>>> phys_enc->hw_intf->idx); > >>>>> > >>>>> Should there be a flush if we are switching from YUV 420 to RGB mode? > >>>> We only need to flush for the sdp packet, but for msa we do not need to > >>>> flush. > >>> > >>> What about having SDP with RGB as colorimetry? In other words, if > >>> there is a decision point, this one looks incorrect. > >>> > >> > >> There are two ways to do it: > >> > >> 1) Use SDP for both RGB and YUV as that supports both. If we implement > >> this policy, then what you are asking for is correct that we will need > >> SDP even to switch back to RGB. But to implement this we will also need > >> to have some sort of state management in the encoder layer about what is > >> the current encoder fmt Vs what is the prev fmt and then trigger > >> peripheral flush only during transitions from RGB to YUV and vice-versa > >> > >> 2) Use SDP only for YUV because MSA does not support YUV formats and use > >> MSA for RGB > >> > >> We decided to implement (2) and there is no significant impact of > >> switching between MSA and SDPs but state management becomes easier. > > > > Yes. However as you wrote, there might be other usecases concerning > > SDP. Having this in mind, it sounds like the driver should decide > > whether to flush peripheral at a different place (when the SDP > > infoframe is being updated?). And the dpu_encoder_phys_vid_enable() > > should use this previous decision. Maybe this should be a part of > > msm_dp_ API, something like msm_dp_needs_peripheral_flush()? > > > > Correct. The decision to flush peripheral or not certainly comes from > the peripheral itself . In this case its DP. > > I think perhaps the usage of hw_cdm here makes it hard to understand > that but the idea behind this was that in the change "drm/msm/dpu: > reserve CDM blocks for DP if mode is YUV420 ", hw_cdm is assigned only > if msm_dp_is_yuv_420_enabled() returns true. Yes. > > So in some sense, the API you are asking for is already > msm_dp_is_yuv_420_enabled() but we can rename that to > msm_dp_needs_periph_flush(). No. This leaks details. We might need peripheral flush for other reasons. So this decision should be made inside the DP driver. BTW, is there a need to flush-peripheral each time the INTF gets enabled? When some bits of configuration were updated (like SDP infoframe or CDM config)? > > The issue here is the phys layer cannot call msm_dp_is_yuv_420_enabled() > so its kind of round-about by checking hw_cdm. Which maybe means that I should finish phys / encoder rework. > > The check is not entirely wrong because for DP, we need hw_cdm for all > YUV formats and not just 420 but the relationship is maybe not clear but > I thought the comment above that would help a bit: > > /* > * Peripheral flush must be updated whenever flushing SDP packets is > needed. > * SDP packets are required for any YUV format (YUV420, YUV422, YUV444). > */ > > The only other way I can think of is maybe we need to introduce a new > phys variable called phys_enc->needs_update_periph_flush and we can set > that in dpu_encoder.c as that can call msm_dp_needs_periph_flush(). > > What do you think of this way? Let me understand first the requirements for peripheral flush (see the questions above). > > >> > >>>>> > >>>>> Also, I'd say, we should move update_pending_flush_periph invocation > >>>>> to this patch. > >>>> Ack > >>>>> > >>>>>> skip_flush: > >>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > >>>>>> b/drivers/gpu/drm/msm/dp/dp_display.c > >>>>>> index 6d764f5b08727..4329435518351 100644 > >>>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c > >>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c > >>>>>> @@ -1399,6 +1399,18 @@ void __exit msm_dp_unregister(void) > >>>>>> platform_driver_unregister(&dp_display_driver); > >>>>>> } > >>>>>> +bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display, > >>>>>> + const struct drm_display_mode *mode) > >>>>>> +{ > >>>>>> + struct dp_display_private *dp; > >>>>>> + const struct drm_display_info *info; > >>>>>> + > >>>>>> + dp = container_of(dp_display, struct dp_display_private, > >>>>>> dp_display); > >>>>>> + info = &dp_display->connector->display_info; > >>>>>> + > >>>>>> + return dp_panel_vsc_sdp_supported(dp->panel) && > >>>>>> drm_mode_is_420_only(info, mode); > >>>>> > >>>>> YUV 420 modes should be filtered out in mode_valid if VSC SDP is not > >>>>> supported. > >>>> Ack. Will change > >>>>> > >>>>>> +} > >>>>>> + > >>>>>> bool msm_dp_wide_bus_available(const struct msm_dp *dp_display) > >>>>>> { > >>>>>> struct dp_display_private *dp; > >>>>>> diff --git a/drivers/gpu/drm/msm/msm_drv.h > >>>>>> b/drivers/gpu/drm/msm/msm_drv.h > >>>>>> index 16a7cbc0b7dd8..b9581bd934e9e 100644 > >>>>>> --- a/drivers/gpu/drm/msm/msm_drv.h > >>>>>> +++ b/drivers/gpu/drm/msm/msm_drv.h > >>>>>> @@ -387,7 +387,8 @@ void __exit msm_dp_unregister(void); > >>>>>> int msm_dp_modeset_init(struct msm_dp *dp_display, struct > >>>>>> drm_device *dev, > >>>>>> struct drm_encoder *encoder); > >>>>>> void msm_dp_snapshot(struct msm_disp_state *disp_state, struct > >>>>>> msm_dp *dp_display); > >>>>>> - > >>>>>> +bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display, > >>>>>> + const struct drm_display_mode *mode); > >>>>>> bool msm_dp_wide_bus_available(const struct msm_dp *dp_display); > >>>>>> #else > >>>>>> @@ -409,6 +410,12 @@ static inline void msm_dp_snapshot(struct > >>>>>> msm_disp_state *disp_state, struct msm > >>>>>> { > >>>>>> } > >>>>>> +static inline bool msm_dp_is_yuv_420_enabled(const struct msm_dp > >>>>>> *dp_display, > >>>>>> + const struct drm_display_mode *mode) > >>>>>> +{ > >>>>>> + return false; > >>>>>> +} > >>>>>> + > >>>>>> static inline bool msm_dp_wide_bus_available(const struct msm_dp > >>>>>> *dp_display) > >>>>>> { > >>>>>> return false; > >>>>> > >>> > >>> > >>> > >>> -- > >>> With best wishes > >>> Dmitry > > > > > >
On 1/28/2024 10:12 PM, Dmitry Baryshkov wrote: > On Mon, 29 Jan 2024 at 07:03, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 1/28/2024 7:42 PM, Dmitry Baryshkov wrote: >>> On Mon, 29 Jan 2024 at 04:58, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >>>> >>>> >>>> >>>> On 1/27/2024 9:55 PM, Dmitry Baryshkov wrote: >>>>> On Sun, 28 Jan 2024 at 07:48, Paloma Arellano <quic_parellan@quicinc.com> wrote: >>>>>> >>>>>> >>>>>> On 1/25/2024 1:57 PM, Dmitry Baryshkov wrote: >>>>>>> On 25/01/2024 21:38, Paloma Arellano wrote: >>>>>>>> Adjust the encoder format programming in the case of video mode for DP >>>>>>>> to accommodate CDM related changes. >>>>>>>> >>>>>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> >>>>>>>> --- >>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 16 +++++++++ >>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 +++++ >>>>>>>> .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 35 ++++++++++++++++--- >>>>>>>> drivers/gpu/drm/msm/dp/dp_display.c | 12 +++++++ >>>>>>>> drivers/gpu/drm/msm/msm_drv.h | 9 ++++- >>>>>>>> 5 files changed, 75 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 b0896814c1562..99ec53446ad21 100644 >>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>>>>>> @@ -222,6 +222,22 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = { >>>>>>>> 15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10 >>>>>>>> }; >>>>>>>> +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, >>>>>>>> const struct drm_display_mode *mode) >>>>>>>> +{ >>>>>>>> + const struct dpu_encoder_virt *dpu_enc; >>>>>>>> + const struct msm_display_info *disp_info; >>>>>>>> + struct msm_drm_private *priv; >>>>>>>> + >>>>>>>> + dpu_enc = to_dpu_encoder_virt(drm_enc); >>>>>>>> + disp_info = &dpu_enc->disp_info; >>>>>>>> + priv = drm_enc->dev->dev_private; >>>>>>>> + >>>>>>>> + if (disp_info->intf_type == INTF_DP && >>>>>>>> + msm_dp_is_yuv_420_enabled(priv->dp[disp_info->h_tile_instance[0]], >>>>>>>> mode)) >>>>>>> >>>>>>> This should not require interacting with DP. If we got here, we must >>>>>>> be sure that 4:2:0 is supported and can be configured. >>>>>> Ack. Will drop this function and only check for if the mode is YUV420. >>>>>>> >>>>>>>> + return DRM_FORMAT_YUV420; >>>>>>>> + >>>>>>>> + return DRM_FORMAT_RGB888; >>>>>>>> +} >>>>>>>> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder >>>>>>>> *drm_enc) >>>>>>>> { >>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>>>>>> index 7b4afa71f1f96..62255d0aa4487 100644 >>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>>>>>> @@ -162,6 +162,14 @@ int dpu_encoder_get_vsync_count(struct >>>>>>>> drm_encoder *drm_enc); >>>>>>>> */ >>>>>>>> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder >>>>>>>> *drm_enc); >>>>>>>> +/** >>>>>>>> + * dpu_encoder_get_drm_fmt - return DRM fourcc format >>>>>>>> + * @drm_enc: Pointer to previously created drm encoder structure >>>>>>>> + * @mode: Corresponding drm_display_mode for dpu encoder >>>>>>>> + */ >>>>>>>> +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, >>>>>>>> + const struct drm_display_mode *mode); >>>>>>>> + >>>>>>>> /** >>>>>>>> * dpu_encoder_get_crc_values_cnt - get number of physical encoders >>>>>>>> contained >>>>>>>> * in virtual encoder that can collect CRC values >>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c >>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c >>>>>>>> index e284bf448bdda..a1dde0ff35dc8 100644 >>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c >>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c >>>>>>>> @@ -234,6 +234,7 @@ static void >>>>>>>> dpu_encoder_phys_vid_setup_timing_engine( >>>>>>>> { >>>>>>>> struct drm_display_mode mode; >>>>>>>> struct dpu_hw_intf_timing_params timing_params = { 0 }; >>>>>>>> + struct dpu_hw_cdm *hw_cdm; >>>>>>>> const struct dpu_format *fmt = NULL; >>>>>>>> u32 fmt_fourcc = DRM_FORMAT_RGB888; >>>>>>>> unsigned long lock_flags; >>>>>>>> @@ -254,17 +255,26 @@ static void >>>>>>>> dpu_encoder_phys_vid_setup_timing_engine( >>>>>>>> DPU_DEBUG_VIDENC(phys_enc, "enabling mode:\n"); >>>>>>>> drm_mode_debug_printmodeline(&mode); >>>>>>>> - if (phys_enc->split_role != ENC_ROLE_SOLO) { >>>>>>>> + hw_cdm = phys_enc->hw_cdm; >>>>>>>> + if (hw_cdm) { >>>>>>>> + intf_cfg.cdm = hw_cdm->idx; >>>>>>>> + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, &mode); >>>>>>>> + } >>>>>>>> + >>>>>>>> + if (phys_enc->split_role != ENC_ROLE_SOLO || >>>>>>>> + dpu_encoder_get_drm_fmt(phys_enc->parent, &mode) == >>>>>>>> DRM_FORMAT_YUV420) { >>>>>>>> mode.hdisplay >>= 1; >>>>>>>> mode.htotal >>= 1; >>>>>>>> mode.hsync_start >>= 1; >>>>>>>> mode.hsync_end >>= 1; >>>>>>>> + mode.hskew >>= 1; >>>>>>> >>>>>>> Separate patch. >>>>>> Ack. >>>>>>> >>>>>>>> DPU_DEBUG_VIDENC(phys_enc, >>>>>>>> - "split_role %d, halve horizontal %d %d %d %d\n", >>>>>>>> + "split_role %d, halve horizontal %d %d %d %d %d\n", >>>>>>>> phys_enc->split_role, >>>>>>>> mode.hdisplay, mode.htotal, >>>>>>>> - mode.hsync_start, mode.hsync_end); >>>>>>>> + mode.hsync_start, mode.hsync_end, >>>>>>>> + mode.hskew); >>>>>>>> } >>>>>>>> drm_mode_to_intf_timing_params(phys_enc, &mode, &timing_params); >>>>>>>> @@ -412,8 +422,15 @@ static int dpu_encoder_phys_vid_control_vblank_irq( >>>>>>>> static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys >>>>>>>> *phys_enc) >>>>>>>> { >>>>>>>> struct dpu_hw_ctl *ctl; >>>>>>>> + struct dpu_hw_cdm *hw_cdm; >>>>>>>> + const struct dpu_format *fmt = NULL; >>>>>>>> + u32 fmt_fourcc = DRM_FORMAT_RGB888; >>>>>>>> ctl = phys_enc->hw_ctl; >>>>>>>> + hw_cdm = phys_enc->hw_cdm; >>>>>>>> + if (hw_cdm) >>>>>>>> + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, >>>>>>>> &phys_enc->cached_mode); >>>>>>>> + fmt = dpu_get_dpu_format(fmt_fourcc); >>>>>>>> DPU_DEBUG_VIDENC(phys_enc, "\n"); >>>>>>>> @@ -422,6 +439,8 @@ static void dpu_encoder_phys_vid_enable(struct >>>>>>>> dpu_encoder_phys *phys_enc) >>>>>>>> dpu_encoder_helper_split_config(phys_enc, >>>>>>>> phys_enc->hw_intf->idx); >>>>>>>> + dpu_encoder_helper_phys_setup_cdm(phys_enc, fmt, >>>>>>>> CDM_CDWN_OUTPUT_HDMI); >>>>>>> >>>>>>> If there is no CDM, why do we need to call this? >>>>>> Inside of dpu_encoder_helper_phys_setup_cdm(), there's a check to see if >>>>>> there is a hw_cdm. If there is not, then it immediately exits the function. >>>>>>> >>>>>>>> + >>>>>>>> dpu_encoder_phys_vid_setup_timing_engine(phys_enc); >>>>>>>> /* >>>>>>>> @@ -437,7 +456,15 @@ static void dpu_encoder_phys_vid_enable(struct >>>>>>>> dpu_encoder_phys *phys_enc) >>>>>>>> if (ctl->ops.update_pending_flush_merge_3d && >>>>>>>> phys_enc->hw_pp->merge_3d) >>>>>>>> ctl->ops.update_pending_flush_merge_3d(ctl, >>>>>>>> phys_enc->hw_pp->merge_3d->idx); >>>>>>>> - if (ctl->ops.update_pending_flush_periph && >>>>>>>> phys_enc->hw_intf->cap->type == INTF_DP) >>>>>>>> + if (ctl->ops.update_pending_flush_cdm && phys_enc->hw_cdm) >>>>>>>> + ctl->ops.update_pending_flush_cdm(ctl, hw_cdm->idx); >>>>>>>> + >>>>>>>> + /* >>>>>>>> + * Peripheral flush must be updated whenever flushing SDP >>>>>>>> packets is needed. >>>>>>>> + * SDP packets are required for any YUV format (YUV420, YUV422, >>>>>>>> YUV444). >>>>>>>> + */ >>>>>>>> + if (ctl->ops.update_pending_flush_periph && >>>>>>>> phys_enc->hw_intf->cap->type == INTF_DP && >>>>>>>> + phys_enc->hw_cdm) >>>>>>>> ctl->ops.update_pending_flush_periph(ctl, >>>>>>>> phys_enc->hw_intf->idx); >>>>>>> >>>>>>> Should there be a flush if we are switching from YUV 420 to RGB mode? >>>>>> We only need to flush for the sdp packet, but for msa we do not need to >>>>>> flush. >>>>> >>>>> What about having SDP with RGB as colorimetry? In other words, if >>>>> there is a decision point, this one looks incorrect. >>>>> >>>> >>>> There are two ways to do it: >>>> >>>> 1) Use SDP for both RGB and YUV as that supports both. If we implement >>>> this policy, then what you are asking for is correct that we will need >>>> SDP even to switch back to RGB. But to implement this we will also need >>>> to have some sort of state management in the encoder layer about what is >>>> the current encoder fmt Vs what is the prev fmt and then trigger >>>> peripheral flush only during transitions from RGB to YUV and vice-versa >>>> >>>> 2) Use SDP only for YUV because MSA does not support YUV formats and use >>>> MSA for RGB >>>> >>>> We decided to implement (2) and there is no significant impact of >>>> switching between MSA and SDPs but state management becomes easier. >>> >>> Yes. However as you wrote, there might be other usecases concerning >>> SDP. Having this in mind, it sounds like the driver should decide >>> whether to flush peripheral at a different place (when the SDP >>> infoframe is being updated?). And the dpu_encoder_phys_vid_enable() >>> should use this previous decision. Maybe this should be a part of >>> msm_dp_ API, something like msm_dp_needs_peripheral_flush()? >>> >> >> Correct. The decision to flush peripheral or not certainly comes from >> the peripheral itself . In this case its DP. >> >> I think perhaps the usage of hw_cdm here makes it hard to understand >> that but the idea behind this was that in the change "drm/msm/dpu: >> reserve CDM blocks for DP if mode is YUV420 ", hw_cdm is assigned only >> if msm_dp_is_yuv_420_enabled() returns true. > > Yes. > >> >> So in some sense, the API you are asking for is already >> msm_dp_is_yuv_420_enabled() but we can rename that to >> msm_dp_needs_periph_flush(). > > No. This leaks details. We might need peripheral flush for other > reasons. So this decision should be made inside the DP driver. > BTW, is there a need to flush-peripheral each time the INTF gets > enabled? When some bits of configuration were updated (like SDP > infoframe or CDM config)? > Not really leaking any details. Interface decides when DPU's flush bit needs to be invoked. All the conditions needing the flush can be absorbed within msm_dp_needs_periph_flush() which will be within DP driver. So I dont see any issue with that. The peripheral flush needs to be invoked whenever SDP/PPS packets need to be sent out at the very least. Those are the two main use-cases so far. Not necessarily each time the INTF is enabled. But .... For PPS, we would typically enable DSC from the first frame itself and we dont support dynamically turning DSC ON/OFF so its a fair assumption that we need to the peripheral flush for DSC only during enable. For SDP, so today we enforce a modeset when we switch from a case of needing cdm / not needing cdm and vice-versa. So even this use-case should be accompanied by a disable()/enable(). So are you suggesting that we unconditionally just do peripheral_flush from enable()? I need to do some checking but maybe .... >> >> The issue here is the phys layer cannot call msm_dp_is_yuv_420_enabled() >> so its kind of round-about by checking hw_cdm. > > Which maybe means that I should finish phys / encoder rework. > Ugh no. Not another rework just for this. >> >> The check is not entirely wrong because for DP, we need hw_cdm for all >> YUV formats and not just 420 but the relationship is maybe not clear but >> I thought the comment above that would help a bit: >> >> /* >> * Peripheral flush must be updated whenever flushing SDP packets is >> needed. >> * SDP packets are required for any YUV format (YUV420, YUV422, YUV444). >> */ >> >> The only other way I can think of is maybe we need to introduce a new >> phys variable called phys_enc->needs_update_periph_flush and we can set >> that in dpu_encoder.c as that can call msm_dp_needs_periph_flush(). >> >> What do you think of this way? > > Let me understand first the requirements for peripheral flush (see the > questions above). > >> >>>> >>>>>>> >>>>>>> Also, I'd say, we should move update_pending_flush_periph invocation >>>>>>> to this patch. >>>>>> Ack >>>>>>> >>>>>>>> skip_flush: >>>>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c >>>>>>>> b/drivers/gpu/drm/msm/dp/dp_display.c >>>>>>>> index 6d764f5b08727..4329435518351 100644 >>>>>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c >>>>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >>>>>>>> @@ -1399,6 +1399,18 @@ void __exit msm_dp_unregister(void) >>>>>>>> platform_driver_unregister(&dp_display_driver); >>>>>>>> } >>>>>>>> +bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display, >>>>>>>> + const struct drm_display_mode *mode) >>>>>>>> +{ >>>>>>>> + struct dp_display_private *dp; >>>>>>>> + const struct drm_display_info *info; >>>>>>>> + >>>>>>>> + dp = container_of(dp_display, struct dp_display_private, >>>>>>>> dp_display); >>>>>>>> + info = &dp_display->connector->display_info; >>>>>>>> + >>>>>>>> + return dp_panel_vsc_sdp_supported(dp->panel) && >>>>>>>> drm_mode_is_420_only(info, mode); >>>>>>> >>>>>>> YUV 420 modes should be filtered out in mode_valid if VSC SDP is not >>>>>>> supported. >>>>>> Ack. Will change >>>>>>> >>>>>>>> +} >>>>>>>> + >>>>>>>> bool msm_dp_wide_bus_available(const struct msm_dp *dp_display) >>>>>>>> { >>>>>>>> struct dp_display_private *dp; >>>>>>>> diff --git a/drivers/gpu/drm/msm/msm_drv.h >>>>>>>> b/drivers/gpu/drm/msm/msm_drv.h >>>>>>>> index 16a7cbc0b7dd8..b9581bd934e9e 100644 >>>>>>>> --- a/drivers/gpu/drm/msm/msm_drv.h >>>>>>>> +++ b/drivers/gpu/drm/msm/msm_drv.h >>>>>>>> @@ -387,7 +387,8 @@ void __exit msm_dp_unregister(void); >>>>>>>> int msm_dp_modeset_init(struct msm_dp *dp_display, struct >>>>>>>> drm_device *dev, >>>>>>>> struct drm_encoder *encoder); >>>>>>>> void msm_dp_snapshot(struct msm_disp_state *disp_state, struct >>>>>>>> msm_dp *dp_display); >>>>>>>> - >>>>>>>> +bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display, >>>>>>>> + const struct drm_display_mode *mode); >>>>>>>> bool msm_dp_wide_bus_available(const struct msm_dp *dp_display); >>>>>>>> #else >>>>>>>> @@ -409,6 +410,12 @@ static inline void msm_dp_snapshot(struct >>>>>>>> msm_disp_state *disp_state, struct msm >>>>>>>> { >>>>>>>> } >>>>>>>> +static inline bool msm_dp_is_yuv_420_enabled(const struct msm_dp >>>>>>>> *dp_display, >>>>>>>> + const struct drm_display_mode *mode) >>>>>>>> +{ >>>>>>>> + return false; >>>>>>>> +} >>>>>>>> + >>>>>>>> static inline bool msm_dp_wide_bus_available(const struct msm_dp >>>>>>>> *dp_display) >>>>>>>> { >>>>>>>> return false; >>>>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> With best wishes >>>>> Dmitry >>> >>> >>> > > >
On Mon, 29 Jan 2024 at 09:08, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > On 1/28/2024 10:12 PM, Dmitry Baryshkov wrote: > > On Mon, 29 Jan 2024 at 07:03, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >> > >> > >> > >> On 1/28/2024 7:42 PM, Dmitry Baryshkov wrote: > >>> On Mon, 29 Jan 2024 at 04:58, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >>>> > >>>> > >>>> > >>>> On 1/27/2024 9:55 PM, Dmitry Baryshkov wrote: > >>>>> On Sun, 28 Jan 2024 at 07:48, Paloma Arellano <quic_parellan@quicinc.com> wrote: > >>>>>> > >>>>>> > >>>>>> On 1/25/2024 1:57 PM, Dmitry Baryshkov wrote: > >>>>>>> On 25/01/2024 21:38, Paloma Arellano wrote: > >>>>>>>> Adjust the encoder format programming in the case of video mode for DP > >>>>>>>> to accommodate CDM related changes. > >>>>>>>> > >>>>>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> > >>>>>>>> --- > >>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 16 +++++++++ > >>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 +++++ > >>>>>>>> .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 35 ++++++++++++++++--- > >>>>>>>> drivers/gpu/drm/msm/dp/dp_display.c | 12 +++++++ > >>>>>>>> drivers/gpu/drm/msm/msm_drv.h | 9 ++++- > >>>>>>>> 5 files changed, 75 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 b0896814c1562..99ec53446ad21 100644 > >>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >>>>>>>> @@ -222,6 +222,22 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = { > >>>>>>>> 15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10 > >>>>>>>> }; > >>>>>>>> +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, > >>>>>>>> const struct drm_display_mode *mode) > >>>>>>>> +{ > >>>>>>>> + const struct dpu_encoder_virt *dpu_enc; > >>>>>>>> + const struct msm_display_info *disp_info; > >>>>>>>> + struct msm_drm_private *priv; > >>>>>>>> + > >>>>>>>> + dpu_enc = to_dpu_encoder_virt(drm_enc); > >>>>>>>> + disp_info = &dpu_enc->disp_info; > >>>>>>>> + priv = drm_enc->dev->dev_private; > >>>>>>>> + > >>>>>>>> + if (disp_info->intf_type == INTF_DP && > >>>>>>>> + msm_dp_is_yuv_420_enabled(priv->dp[disp_info->h_tile_instance[0]], > >>>>>>>> mode)) > >>>>>>> > >>>>>>> This should not require interacting with DP. If we got here, we must > >>>>>>> be sure that 4:2:0 is supported and can be configured. > >>>>>> Ack. Will drop this function and only check for if the mode is YUV420. > >>>>>>> > >>>>>>>> + return DRM_FORMAT_YUV420; > >>>>>>>> + > >>>>>>>> + return DRM_FORMAT_RGB888; > >>>>>>>> +} > >>>>>>>> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder > >>>>>>>> *drm_enc) > >>>>>>>> { > >>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >>>>>>>> index 7b4afa71f1f96..62255d0aa4487 100644 > >>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >>>>>>>> @@ -162,6 +162,14 @@ int dpu_encoder_get_vsync_count(struct > >>>>>>>> drm_encoder *drm_enc); > >>>>>>>> */ > >>>>>>>> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder > >>>>>>>> *drm_enc); > >>>>>>>> +/** > >>>>>>>> + * dpu_encoder_get_drm_fmt - return DRM fourcc format > >>>>>>>> + * @drm_enc: Pointer to previously created drm encoder structure > >>>>>>>> + * @mode: Corresponding drm_display_mode for dpu encoder > >>>>>>>> + */ > >>>>>>>> +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, > >>>>>>>> + const struct drm_display_mode *mode); > >>>>>>>> + > >>>>>>>> /** > >>>>>>>> * dpu_encoder_get_crc_values_cnt - get number of physical encoders > >>>>>>>> contained > >>>>>>>> * in virtual encoder that can collect CRC values > >>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > >>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > >>>>>>>> index e284bf448bdda..a1dde0ff35dc8 100644 > >>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > >>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > >>>>>>>> @@ -234,6 +234,7 @@ static void > >>>>>>>> dpu_encoder_phys_vid_setup_timing_engine( > >>>>>>>> { > >>>>>>>> struct drm_display_mode mode; > >>>>>>>> struct dpu_hw_intf_timing_params timing_params = { 0 }; > >>>>>>>> + struct dpu_hw_cdm *hw_cdm; > >>>>>>>> const struct dpu_format *fmt = NULL; > >>>>>>>> u32 fmt_fourcc = DRM_FORMAT_RGB888; > >>>>>>>> unsigned long lock_flags; > >>>>>>>> @@ -254,17 +255,26 @@ static void > >>>>>>>> dpu_encoder_phys_vid_setup_timing_engine( > >>>>>>>> DPU_DEBUG_VIDENC(phys_enc, "enabling mode:\n"); > >>>>>>>> drm_mode_debug_printmodeline(&mode); > >>>>>>>> - if (phys_enc->split_role != ENC_ROLE_SOLO) { > >>>>>>>> + hw_cdm = phys_enc->hw_cdm; > >>>>>>>> + if (hw_cdm) { > >>>>>>>> + intf_cfg.cdm = hw_cdm->idx; > >>>>>>>> + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, &mode); > >>>>>>>> + } > >>>>>>>> + > >>>>>>>> + if (phys_enc->split_role != ENC_ROLE_SOLO || > >>>>>>>> + dpu_encoder_get_drm_fmt(phys_enc->parent, &mode) == > >>>>>>>> DRM_FORMAT_YUV420) { > >>>>>>>> mode.hdisplay >>= 1; > >>>>>>>> mode.htotal >>= 1; > >>>>>>>> mode.hsync_start >>= 1; > >>>>>>>> mode.hsync_end >>= 1; > >>>>>>>> + mode.hskew >>= 1; > >>>>>>> > >>>>>>> Separate patch. > >>>>>> Ack. > >>>>>>> > >>>>>>>> DPU_DEBUG_VIDENC(phys_enc, > >>>>>>>> - "split_role %d, halve horizontal %d %d %d %d\n", > >>>>>>>> + "split_role %d, halve horizontal %d %d %d %d %d\n", > >>>>>>>> phys_enc->split_role, > >>>>>>>> mode.hdisplay, mode.htotal, > >>>>>>>> - mode.hsync_start, mode.hsync_end); > >>>>>>>> + mode.hsync_start, mode.hsync_end, > >>>>>>>> + mode.hskew); > >>>>>>>> } > >>>>>>>> drm_mode_to_intf_timing_params(phys_enc, &mode, &timing_params); > >>>>>>>> @@ -412,8 +422,15 @@ static int dpu_encoder_phys_vid_control_vblank_irq( > >>>>>>>> static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys > >>>>>>>> *phys_enc) > >>>>>>>> { > >>>>>>>> struct dpu_hw_ctl *ctl; > >>>>>>>> + struct dpu_hw_cdm *hw_cdm; > >>>>>>>> + const struct dpu_format *fmt = NULL; > >>>>>>>> + u32 fmt_fourcc = DRM_FORMAT_RGB888; > >>>>>>>> ctl = phys_enc->hw_ctl; > >>>>>>>> + hw_cdm = phys_enc->hw_cdm; > >>>>>>>> + if (hw_cdm) > >>>>>>>> + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, > >>>>>>>> &phys_enc->cached_mode); > >>>>>>>> + fmt = dpu_get_dpu_format(fmt_fourcc); > >>>>>>>> DPU_DEBUG_VIDENC(phys_enc, "\n"); > >>>>>>>> @@ -422,6 +439,8 @@ static void dpu_encoder_phys_vid_enable(struct > >>>>>>>> dpu_encoder_phys *phys_enc) > >>>>>>>> dpu_encoder_helper_split_config(phys_enc, > >>>>>>>> phys_enc->hw_intf->idx); > >>>>>>>> + dpu_encoder_helper_phys_setup_cdm(phys_enc, fmt, > >>>>>>>> CDM_CDWN_OUTPUT_HDMI); > >>>>>>> > >>>>>>> If there is no CDM, why do we need to call this? > >>>>>> Inside of dpu_encoder_helper_phys_setup_cdm(), there's a check to see if > >>>>>> there is a hw_cdm. If there is not, then it immediately exits the function. > >>>>>>> > >>>>>>>> + > >>>>>>>> dpu_encoder_phys_vid_setup_timing_engine(phys_enc); > >>>>>>>> /* > >>>>>>>> @@ -437,7 +456,15 @@ static void dpu_encoder_phys_vid_enable(struct > >>>>>>>> dpu_encoder_phys *phys_enc) > >>>>>>>> if (ctl->ops.update_pending_flush_merge_3d && > >>>>>>>> phys_enc->hw_pp->merge_3d) > >>>>>>>> ctl->ops.update_pending_flush_merge_3d(ctl, > >>>>>>>> phys_enc->hw_pp->merge_3d->idx); > >>>>>>>> - if (ctl->ops.update_pending_flush_periph && > >>>>>>>> phys_enc->hw_intf->cap->type == INTF_DP) > >>>>>>>> + if (ctl->ops.update_pending_flush_cdm && phys_enc->hw_cdm) > >>>>>>>> + ctl->ops.update_pending_flush_cdm(ctl, hw_cdm->idx); > >>>>>>>> + > >>>>>>>> + /* > >>>>>>>> + * Peripheral flush must be updated whenever flushing SDP > >>>>>>>> packets is needed. > >>>>>>>> + * SDP packets are required for any YUV format (YUV420, YUV422, > >>>>>>>> YUV444). > >>>>>>>> + */ > >>>>>>>> + if (ctl->ops.update_pending_flush_periph && > >>>>>>>> phys_enc->hw_intf->cap->type == INTF_DP && > >>>>>>>> + phys_enc->hw_cdm) > >>>>>>>> ctl->ops.update_pending_flush_periph(ctl, > >>>>>>>> phys_enc->hw_intf->idx); > >>>>>>> > >>>>>>> Should there be a flush if we are switching from YUV 420 to RGB mode? > >>>>>> We only need to flush for the sdp packet, but for msa we do not need to > >>>>>> flush. > >>>>> > >>>>> What about having SDP with RGB as colorimetry? In other words, if > >>>>> there is a decision point, this one looks incorrect. > >>>>> > >>>> > >>>> There are two ways to do it: > >>>> > >>>> 1) Use SDP for both RGB and YUV as that supports both. If we implement > >>>> this policy, then what you are asking for is correct that we will need > >>>> SDP even to switch back to RGB. But to implement this we will also need > >>>> to have some sort of state management in the encoder layer about what is > >>>> the current encoder fmt Vs what is the prev fmt and then trigger > >>>> peripheral flush only during transitions from RGB to YUV and vice-versa > >>>> > >>>> 2) Use SDP only for YUV because MSA does not support YUV formats and use > >>>> MSA for RGB > >>>> > >>>> We decided to implement (2) and there is no significant impact of > >>>> switching between MSA and SDPs but state management becomes easier. > >>> > >>> Yes. However as you wrote, there might be other usecases concerning > >>> SDP. Having this in mind, it sounds like the driver should decide > >>> whether to flush peripheral at a different place (when the SDP > >>> infoframe is being updated?). And the dpu_encoder_phys_vid_enable() > >>> should use this previous decision. Maybe this should be a part of > >>> msm_dp_ API, something like msm_dp_needs_peripheral_flush()? > >>> > >> > >> Correct. The decision to flush peripheral or not certainly comes from > >> the peripheral itself . In this case its DP. > >> > >> I think perhaps the usage of hw_cdm here makes it hard to understand > >> that but the idea behind this was that in the change "drm/msm/dpu: > >> reserve CDM blocks for DP if mode is YUV420 ", hw_cdm is assigned only > >> if msm_dp_is_yuv_420_enabled() returns true. > > > > Yes. > > > >> > >> So in some sense, the API you are asking for is already > >> msm_dp_is_yuv_420_enabled() but we can rename that to > >> msm_dp_needs_periph_flush(). > > > > No. This leaks details. We might need peripheral flush for other > > reasons. So this decision should be made inside the DP driver. > > BTW, is there a need to flush-peripheral each time the INTF gets > > enabled? When some bits of configuration were updated (like SDP > > infoframe or CDM config)? > > > > Not really leaking any details. Interface decides when DPU's flush bit > needs to be invoked. All the conditions needing the flush can be > absorbed within msm_dp_needs_periph_flush() which will be within DP > driver. So I dont see any issue with that. As long as we have two distinct functions: msm_dp_needs_periph_flush() and msm_dp_is_yuv420_enabled(), it's fine with me. > > The peripheral flush needs to be invoked whenever SDP/PPS packets need > to be sent out at the very least. Those are the two main use-cases so far. > > Not necessarily each time the INTF is enabled. > > But .... > > For PPS, we would typically enable DSC from the first frame itself and > we dont support dynamically turning DSC ON/OFF so its a fair assumption > that we need to the peripheral flush for DSC only during enable. > > For SDP, so today we enforce a modeset when we switch from a case of > needing cdm / not needing cdm and vice-versa. So even this use-case > should be accompanied by a disable()/enable(). My point was that there might be other changes to the SDP infoframe. > > So are you suggesting that we unconditionally just do peripheral_flush > from enable()? > > I need to do some checking but maybe .... This might be an option too. I don't know what delays / issues it can cause. > > >> > >> The issue here is the phys layer cannot call msm_dp_is_yuv_420_enabled() > >> so its kind of round-about by checking hw_cdm. > > > > Which maybe means that I should finish phys / encoder rework. > > > > Ugh no. Not another rework just for this. It is pending for the CTL flush story. > > >> > >> The check is not entirely wrong because for DP, we need hw_cdm for all > >> YUV formats and not just 420 but the relationship is maybe not clear but > >> I thought the comment above that would help a bit: > >> > >> /* > >> * Peripheral flush must be updated whenever flushing SDP packets is > >> needed. > >> * SDP packets are required for any YUV format (YUV420, YUV422, YUV444). > >> */ > >> > >> The only other way I can think of is maybe we need to introduce a new > >> phys variable called phys_enc->needs_update_periph_flush and we can set > >> that in dpu_encoder.c as that can call msm_dp_needs_periph_flush(). > >> > >> What do you think of this way? > > > > Let me understand first the requirements for peripheral flush (see the > > questions above). > > > >> > >>>> > >>>>>>> > >>>>>>> Also, I'd say, we should move update_pending_flush_periph invocation > >>>>>>> to this patch. > >>>>>> Ack > >>>>>>> > >>>>>>>> skip_flush: > >>>>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > >>>>>>>> b/drivers/gpu/drm/msm/dp/dp_display.c > >>>>>>>> index 6d764f5b08727..4329435518351 100644 > >>>>>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c > >>>>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c > >>>>>>>> @@ -1399,6 +1399,18 @@ void __exit msm_dp_unregister(void) > >>>>>>>> platform_driver_unregister(&dp_display_driver); > >>>>>>>> } > >>>>>>>> +bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display, > >>>>>>>> + const struct drm_display_mode *mode) > >>>>>>>> +{ > >>>>>>>> + struct dp_display_private *dp; > >>>>>>>> + const struct drm_display_info *info; > >>>>>>>> + > >>>>>>>> + dp = container_of(dp_display, struct dp_display_private, > >>>>>>>> dp_display); > >>>>>>>> + info = &dp_display->connector->display_info; > >>>>>>>> + > >>>>>>>> + return dp_panel_vsc_sdp_supported(dp->panel) && > >>>>>>>> drm_mode_is_420_only(info, mode); > >>>>>>> > >>>>>>> YUV 420 modes should be filtered out in mode_valid if VSC SDP is not > >>>>>>> supported. > >>>>>> Ack. Will change > >>>>>>> > >>>>>>>> +} > >>>>>>>> + > >>>>>>>> bool msm_dp_wide_bus_available(const struct msm_dp *dp_display) > >>>>>>>> { > >>>>>>>> struct dp_display_private *dp; > >>>>>>>> diff --git a/drivers/gpu/drm/msm/msm_drv.h > >>>>>>>> b/drivers/gpu/drm/msm/msm_drv.h > >>>>>>>> index 16a7cbc0b7dd8..b9581bd934e9e 100644 > >>>>>>>> --- a/drivers/gpu/drm/msm/msm_drv.h > >>>>>>>> +++ b/drivers/gpu/drm/msm/msm_drv.h > >>>>>>>> @@ -387,7 +387,8 @@ void __exit msm_dp_unregister(void); > >>>>>>>> int msm_dp_modeset_init(struct msm_dp *dp_display, struct > >>>>>>>> drm_device *dev, > >>>>>>>> struct drm_encoder *encoder); > >>>>>>>> void msm_dp_snapshot(struct msm_disp_state *disp_state, struct > >>>>>>>> msm_dp *dp_display); > >>>>>>>> - > >>>>>>>> +bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display, > >>>>>>>> + const struct drm_display_mode *mode); > >>>>>>>> bool msm_dp_wide_bus_available(const struct msm_dp *dp_display); > >>>>>>>> #else > >>>>>>>> @@ -409,6 +410,12 @@ static inline void msm_dp_snapshot(struct > >>>>>>>> msm_disp_state *disp_state, struct msm > >>>>>>>> { > >>>>>>>> } > >>>>>>>> +static inline bool msm_dp_is_yuv_420_enabled(const struct msm_dp > >>>>>>>> *dp_display, > >>>>>>>> + const struct drm_display_mode *mode) > >>>>>>>> +{ > >>>>>>>> + return false; > >>>>>>>> +} > >>>>>>>> + > >>>>>>>> static inline bool msm_dp_wide_bus_available(const struct msm_dp > >>>>>>>> *dp_display) > >>>>>>>> { > >>>>>>>> return false; > >>>>>>> > >>>>> > >>>>> > >>>>> > >>>>> -- > >>>>> With best wishes > >>>>> Dmitry > >>> > >>> > >>> > > > > > > -- With best wishes Dmitry
On 1/29/2024 3:44 PM, Dmitry Baryshkov wrote: > On Mon, 29 Jan 2024 at 09:08, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> On 1/28/2024 10:12 PM, Dmitry Baryshkov wrote: >>> On Mon, 29 Jan 2024 at 07:03, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >>>> >>>> >>>> >>>> On 1/28/2024 7:42 PM, Dmitry Baryshkov wrote: >>>>> On Mon, 29 Jan 2024 at 04:58, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 1/27/2024 9:55 PM, Dmitry Baryshkov wrote: >>>>>>> On Sun, 28 Jan 2024 at 07:48, Paloma Arellano <quic_parellan@quicinc.com> wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 1/25/2024 1:57 PM, Dmitry Baryshkov wrote: >>>>>>>>> On 25/01/2024 21:38, Paloma Arellano wrote: >>>>>>>>>> Adjust the encoder format programming in the case of video mode for DP >>>>>>>>>> to accommodate CDM related changes. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> >>>>>>>>>> --- >>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 16 +++++++++ >>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 +++++ >>>>>>>>>> .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 35 ++++++++++++++++--- >>>>>>>>>> drivers/gpu/drm/msm/dp/dp_display.c | 12 +++++++ >>>>>>>>>> drivers/gpu/drm/msm/msm_drv.h | 9 ++++- >>>>>>>>>> 5 files changed, 75 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 b0896814c1562..99ec53446ad21 100644 >>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>>>>>>>> @@ -222,6 +222,22 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = { >>>>>>>>>> 15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10 >>>>>>>>>> }; >>>>>>>>>> +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, >>>>>>>>>> const struct drm_display_mode *mode) >>>>>>>>>> +{ >>>>>>>>>> + const struct dpu_encoder_virt *dpu_enc; >>>>>>>>>> + const struct msm_display_info *disp_info; >>>>>>>>>> + struct msm_drm_private *priv; >>>>>>>>>> + >>>>>>>>>> + dpu_enc = to_dpu_encoder_virt(drm_enc); >>>>>>>>>> + disp_info = &dpu_enc->disp_info; >>>>>>>>>> + priv = drm_enc->dev->dev_private; >>>>>>>>>> + >>>>>>>>>> + if (disp_info->intf_type == INTF_DP && >>>>>>>>>> + msm_dp_is_yuv_420_enabled(priv->dp[disp_info->h_tile_instance[0]], >>>>>>>>>> mode)) >>>>>>>>> >>>>>>>>> This should not require interacting with DP. If we got here, we must >>>>>>>>> be sure that 4:2:0 is supported and can be configured. >>>>>>>> Ack. Will drop this function and only check for if the mode is YUV420. >>>>>>>>> >>>>>>>>>> + return DRM_FORMAT_YUV420; >>>>>>>>>> + >>>>>>>>>> + return DRM_FORMAT_RGB888; >>>>>>>>>> +} >>>>>>>>>> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder >>>>>>>>>> *drm_enc) >>>>>>>>>> { >>>>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>>>>>>>> index 7b4afa71f1f96..62255d0aa4487 100644 >>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>>>>>>>> @@ -162,6 +162,14 @@ int dpu_encoder_get_vsync_count(struct >>>>>>>>>> drm_encoder *drm_enc); >>>>>>>>>> */ >>>>>>>>>> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder >>>>>>>>>> *drm_enc); >>>>>>>>>> +/** >>>>>>>>>> + * dpu_encoder_get_drm_fmt - return DRM fourcc format >>>>>>>>>> + * @drm_enc: Pointer to previously created drm encoder structure >>>>>>>>>> + * @mode: Corresponding drm_display_mode for dpu encoder >>>>>>>>>> + */ >>>>>>>>>> +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, >>>>>>>>>> + const struct drm_display_mode *mode); >>>>>>>>>> + >>>>>>>>>> /** >>>>>>>>>> * dpu_encoder_get_crc_values_cnt - get number of physical encoders >>>>>>>>>> contained >>>>>>>>>> * in virtual encoder that can collect CRC values >>>>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c >>>>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c >>>>>>>>>> index e284bf448bdda..a1dde0ff35dc8 100644 >>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c >>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c >>>>>>>>>> @@ -234,6 +234,7 @@ static void >>>>>>>>>> dpu_encoder_phys_vid_setup_timing_engine( >>>>>>>>>> { >>>>>>>>>> struct drm_display_mode mode; >>>>>>>>>> struct dpu_hw_intf_timing_params timing_params = { 0 }; >>>>>>>>>> + struct dpu_hw_cdm *hw_cdm; >>>>>>>>>> const struct dpu_format *fmt = NULL; >>>>>>>>>> u32 fmt_fourcc = DRM_FORMAT_RGB888; >>>>>>>>>> unsigned long lock_flags; >>>>>>>>>> @@ -254,17 +255,26 @@ static void >>>>>>>>>> dpu_encoder_phys_vid_setup_timing_engine( >>>>>>>>>> DPU_DEBUG_VIDENC(phys_enc, "enabling mode:\n"); >>>>>>>>>> drm_mode_debug_printmodeline(&mode); >>>>>>>>>> - if (phys_enc->split_role != ENC_ROLE_SOLO) { >>>>>>>>>> + hw_cdm = phys_enc->hw_cdm; >>>>>>>>>> + if (hw_cdm) { >>>>>>>>>> + intf_cfg.cdm = hw_cdm->idx; >>>>>>>>>> + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, &mode); >>>>>>>>>> + } >>>>>>>>>> + >>>>>>>>>> + if (phys_enc->split_role != ENC_ROLE_SOLO || >>>>>>>>>> + dpu_encoder_get_drm_fmt(phys_enc->parent, &mode) == >>>>>>>>>> DRM_FORMAT_YUV420) { >>>>>>>>>> mode.hdisplay >>= 1; >>>>>>>>>> mode.htotal >>= 1; >>>>>>>>>> mode.hsync_start >>= 1; >>>>>>>>>> mode.hsync_end >>= 1; >>>>>>>>>> + mode.hskew >>= 1; >>>>>>>>> >>>>>>>>> Separate patch. >>>>>>>> Ack. >>>>>>>>> >>>>>>>>>> DPU_DEBUG_VIDENC(phys_enc, >>>>>>>>>> - "split_role %d, halve horizontal %d %d %d %d\n", >>>>>>>>>> + "split_role %d, halve horizontal %d %d %d %d %d\n", >>>>>>>>>> phys_enc->split_role, >>>>>>>>>> mode.hdisplay, mode.htotal, >>>>>>>>>> - mode.hsync_start, mode.hsync_end); >>>>>>>>>> + mode.hsync_start, mode.hsync_end, >>>>>>>>>> + mode.hskew); >>>>>>>>>> } >>>>>>>>>> drm_mode_to_intf_timing_params(phys_enc, &mode, &timing_params); >>>>>>>>>> @@ -412,8 +422,15 @@ static int dpu_encoder_phys_vid_control_vblank_irq( >>>>>>>>>> static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys >>>>>>>>>> *phys_enc) >>>>>>>>>> { >>>>>>>>>> struct dpu_hw_ctl *ctl; >>>>>>>>>> + struct dpu_hw_cdm *hw_cdm; >>>>>>>>>> + const struct dpu_format *fmt = NULL; >>>>>>>>>> + u32 fmt_fourcc = DRM_FORMAT_RGB888; >>>>>>>>>> ctl = phys_enc->hw_ctl; >>>>>>>>>> + hw_cdm = phys_enc->hw_cdm; >>>>>>>>>> + if (hw_cdm) >>>>>>>>>> + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, >>>>>>>>>> &phys_enc->cached_mode); >>>>>>>>>> + fmt = dpu_get_dpu_format(fmt_fourcc); >>>>>>>>>> DPU_DEBUG_VIDENC(phys_enc, "\n"); >>>>>>>>>> @@ -422,6 +439,8 @@ static void dpu_encoder_phys_vid_enable(struct >>>>>>>>>> dpu_encoder_phys *phys_enc) >>>>>>>>>> dpu_encoder_helper_split_config(phys_enc, >>>>>>>>>> phys_enc->hw_intf->idx); >>>>>>>>>> + dpu_encoder_helper_phys_setup_cdm(phys_enc, fmt, >>>>>>>>>> CDM_CDWN_OUTPUT_HDMI); >>>>>>>>> >>>>>>>>> If there is no CDM, why do we need to call this? >>>>>>>> Inside of dpu_encoder_helper_phys_setup_cdm(), there's a check to see if >>>>>>>> there is a hw_cdm. If there is not, then it immediately exits the function. >>>>>>>>> >>>>>>>>>> + >>>>>>>>>> dpu_encoder_phys_vid_setup_timing_engine(phys_enc); >>>>>>>>>> /* >>>>>>>>>> @@ -437,7 +456,15 @@ static void dpu_encoder_phys_vid_enable(struct >>>>>>>>>> dpu_encoder_phys *phys_enc) >>>>>>>>>> if (ctl->ops.update_pending_flush_merge_3d && >>>>>>>>>> phys_enc->hw_pp->merge_3d) >>>>>>>>>> ctl->ops.update_pending_flush_merge_3d(ctl, >>>>>>>>>> phys_enc->hw_pp->merge_3d->idx); >>>>>>>>>> - if (ctl->ops.update_pending_flush_periph && >>>>>>>>>> phys_enc->hw_intf->cap->type == INTF_DP) >>>>>>>>>> + if (ctl->ops.update_pending_flush_cdm && phys_enc->hw_cdm) >>>>>>>>>> + ctl->ops.update_pending_flush_cdm(ctl, hw_cdm->idx); >>>>>>>>>> + >>>>>>>>>> + /* >>>>>>>>>> + * Peripheral flush must be updated whenever flushing SDP >>>>>>>>>> packets is needed. >>>>>>>>>> + * SDP packets are required for any YUV format (YUV420, YUV422, >>>>>>>>>> YUV444). >>>>>>>>>> + */ >>>>>>>>>> + if (ctl->ops.update_pending_flush_periph && >>>>>>>>>> phys_enc->hw_intf->cap->type == INTF_DP && >>>>>>>>>> + phys_enc->hw_cdm) >>>>>>>>>> ctl->ops.update_pending_flush_periph(ctl, >>>>>>>>>> phys_enc->hw_intf->idx); >>>>>>>>> >>>>>>>>> Should there be a flush if we are switching from YUV 420 to RGB mode? >>>>>>>> We only need to flush for the sdp packet, but for msa we do not need to >>>>>>>> flush. >>>>>>> >>>>>>> What about having SDP with RGB as colorimetry? In other words, if >>>>>>> there is a decision point, this one looks incorrect. >>>>>>> >>>>>> >>>>>> There are two ways to do it: >>>>>> >>>>>> 1) Use SDP for both RGB and YUV as that supports both. If we implement >>>>>> this policy, then what you are asking for is correct that we will need >>>>>> SDP even to switch back to RGB. But to implement this we will also need >>>>>> to have some sort of state management in the encoder layer about what is >>>>>> the current encoder fmt Vs what is the prev fmt and then trigger >>>>>> peripheral flush only during transitions from RGB to YUV and vice-versa >>>>>> >>>>>> 2) Use SDP only for YUV because MSA does not support YUV formats and use >>>>>> MSA for RGB >>>>>> >>>>>> We decided to implement (2) and there is no significant impact of >>>>>> switching between MSA and SDPs but state management becomes easier. >>>>> >>>>> Yes. However as you wrote, there might be other usecases concerning >>>>> SDP. Having this in mind, it sounds like the driver should decide >>>>> whether to flush peripheral at a different place (when the SDP >>>>> infoframe is being updated?). And the dpu_encoder_phys_vid_enable() >>>>> should use this previous decision. Maybe this should be a part of >>>>> msm_dp_ API, something like msm_dp_needs_peripheral_flush()? >>>>> >>>> >>>> Correct. The decision to flush peripheral or not certainly comes from >>>> the peripheral itself . In this case its DP. >>>> >>>> I think perhaps the usage of hw_cdm here makes it hard to understand >>>> that but the idea behind this was that in the change "drm/msm/dpu: >>>> reserve CDM blocks for DP if mode is YUV420 ", hw_cdm is assigned only >>>> if msm_dp_is_yuv_420_enabled() returns true. >>> >>> Yes. >>> >>>> >>>> So in some sense, the API you are asking for is already >>>> msm_dp_is_yuv_420_enabled() but we can rename that to >>>> msm_dp_needs_periph_flush(). >>> >>> No. This leaks details. We might need peripheral flush for other >>> reasons. So this decision should be made inside the DP driver. >>> BTW, is there a need to flush-peripheral each time the INTF gets >>> enabled? When some bits of configuration were updated (like SDP >>> infoframe or CDM config)? >>> >> >> Not really leaking any details. Interface decides when DPU's flush bit >> needs to be invoked. All the conditions needing the flush can be >> absorbed within msm_dp_needs_periph_flush() which will be within DP >> driver. So I dont see any issue with that. > > As long as we have two distinct functions: msm_dp_needs_periph_flush() > and msm_dp_is_yuv420_enabled(), it's fine with me. > I am okay with it being distinct but in this series and till DP DSC is posted, we have only YUV420 SDP use-case for peripheral flush. So are you okay with the fact that the code will look something like bool msm_dp_needs_periph_flush() { return msm_dp_is_yuv420_enabled(); } >> >> The peripheral flush needs to be invoked whenever SDP/PPS packets need >> to be sent out at the very least. Those are the two main use-cases so far. >> >> Not necessarily each time the INTF is enabled. >> >> But .... >> >> For PPS, we would typically enable DSC from the first frame itself and >> we dont support dynamically turning DSC ON/OFF so its a fair assumption >> that we need to the peripheral flush for DSC only during enable. >> >> For SDP, so today we enforce a modeset when we switch from a case of >> needing cdm / not needing cdm and vice-versa. So even this use-case >> should be accompanied by a disable()/enable(). > > My point was that there might be other changes to the SDP infoframe. > >> >> So are you suggesting that we unconditionally just do peripheral_flush >> from enable()? >> >> I need to do some checking but maybe .... > > This might be an option too. I don't know what delays / issues it can cause. > I will check this if the other option we discussed above does not work for you. >> >>>> >>>> The issue here is the phys layer cannot call msm_dp_is_yuv_420_enabled() >>>> so its kind of round-about by checking hw_cdm. >>> >>> Which maybe means that I should finish phys / encoder rework. >>> >> >> Ugh no. Not another rework just for this. > > It is pending for the CTL flush story. > >> >>>> >>>> The check is not entirely wrong because for DP, we need hw_cdm for all >>>> YUV formats and not just 420 but the relationship is maybe not clear but >>>> I thought the comment above that would help a bit: >>>> >>>> /* >>>> * Peripheral flush must be updated whenever flushing SDP packets is >>>> needed. >>>> * SDP packets are required for any YUV format (YUV420, YUV422, YUV444). >>>> */ >>>> >>>> The only other way I can think of is maybe we need to introduce a new >>>> phys variable called phys_enc->needs_update_periph_flush and we can set >>>> that in dpu_encoder.c as that can call msm_dp_needs_periph_flush(). >>>> >>>> What do you think of this way? >>> >>> Let me understand first the requirements for peripheral flush (see the >>> questions above). >>> >>>> >>>>>> >>>>>>>>> >>>>>>>>> Also, I'd say, we should move update_pending_flush_periph invocation >>>>>>>>> to this patch. >>>>>>>> Ack >>>>>>>>> >>>>>>>>>> skip_flush: >>>>>>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c >>>>>>>>>> b/drivers/gpu/drm/msm/dp/dp_display.c >>>>>>>>>> index 6d764f5b08727..4329435518351 100644 >>>>>>>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c >>>>>>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >>>>>>>>>> @@ -1399,6 +1399,18 @@ void __exit msm_dp_unregister(void) >>>>>>>>>> platform_driver_unregister(&dp_display_driver); >>>>>>>>>> } >>>>>>>>>> +bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display, >>>>>>>>>> + const struct drm_display_mode *mode) >>>>>>>>>> +{ >>>>>>>>>> + struct dp_display_private *dp; >>>>>>>>>> + const struct drm_display_info *info; >>>>>>>>>> + >>>>>>>>>> + dp = container_of(dp_display, struct dp_display_private, >>>>>>>>>> dp_display); >>>>>>>>>> + info = &dp_display->connector->display_info; >>>>>>>>>> + >>>>>>>>>> + return dp_panel_vsc_sdp_supported(dp->panel) && >>>>>>>>>> drm_mode_is_420_only(info, mode); >>>>>>>>> >>>>>>>>> YUV 420 modes should be filtered out in mode_valid if VSC SDP is not >>>>>>>>> supported. >>>>>>>> Ack. Will change >>>>>>>>> >>>>>>>>>> +} >>>>>>>>>> + >>>>>>>>>> bool msm_dp_wide_bus_available(const struct msm_dp *dp_display) >>>>>>>>>> { >>>>>>>>>> struct dp_display_private *dp; >>>>>>>>>> diff --git a/drivers/gpu/drm/msm/msm_drv.h >>>>>>>>>> b/drivers/gpu/drm/msm/msm_drv.h >>>>>>>>>> index 16a7cbc0b7dd8..b9581bd934e9e 100644 >>>>>>>>>> --- a/drivers/gpu/drm/msm/msm_drv.h >>>>>>>>>> +++ b/drivers/gpu/drm/msm/msm_drv.h >>>>>>>>>> @@ -387,7 +387,8 @@ void __exit msm_dp_unregister(void); >>>>>>>>>> int msm_dp_modeset_init(struct msm_dp *dp_display, struct >>>>>>>>>> drm_device *dev, >>>>>>>>>> struct drm_encoder *encoder); >>>>>>>>>> void msm_dp_snapshot(struct msm_disp_state *disp_state, struct >>>>>>>>>> msm_dp *dp_display); >>>>>>>>>> - >>>>>>>>>> +bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display, >>>>>>>>>> + const struct drm_display_mode *mode); >>>>>>>>>> bool msm_dp_wide_bus_available(const struct msm_dp *dp_display); >>>>>>>>>> #else >>>>>>>>>> @@ -409,6 +410,12 @@ static inline void msm_dp_snapshot(struct >>>>>>>>>> msm_disp_state *disp_state, struct msm >>>>>>>>>> { >>>>>>>>>> } >>>>>>>>>> +static inline bool msm_dp_is_yuv_420_enabled(const struct msm_dp >>>>>>>>>> *dp_display, >>>>>>>>>> + const struct drm_display_mode *mode) >>>>>>>>>> +{ >>>>>>>>>> + return false; >>>>>>>>>> +} >>>>>>>>>> + >>>>>>>>>> static inline bool msm_dp_wide_bus_available(const struct msm_dp >>>>>>>>>> *dp_display) >>>>>>>>>> { >>>>>>>>>> return false; >>>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> With best wishes >>>>>>> Dmitry >>>>> >>>>> >>>>> >>> >>> >>> > > > > -- > With best wishes > Dmitry
On Thu, 1 Feb 2024 at 03:30, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 1/29/2024 3:44 PM, Dmitry Baryshkov wrote: > > On Mon, 29 Jan 2024 at 09:08, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >> > >> On 1/28/2024 10:12 PM, Dmitry Baryshkov wrote: > >>> On Mon, 29 Jan 2024 at 07:03, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >>>> > >>>> > >>>> > >>>> On 1/28/2024 7:42 PM, Dmitry Baryshkov wrote: > >>>>> On Mon, 29 Jan 2024 at 04:58, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >>>>>> > >>>>>> > >>>>>> > >>>>>> On 1/27/2024 9:55 PM, Dmitry Baryshkov wrote: > >>>>>>> On Sun, 28 Jan 2024 at 07:48, Paloma Arellano <quic_parellan@quicinc.com> wrote: > >>>>>>>> > >>>>>>>> > >>>>>>>> On 1/25/2024 1:57 PM, Dmitry Baryshkov wrote: > >>>>>>>>> On 25/01/2024 21:38, Paloma Arellano wrote: > >>>>>>>>>> Adjust the encoder format programming in the case of video mode for DP > >>>>>>>>>> to accommodate CDM related changes. > >>>>>>>>>> > >>>>>>>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> > >>>>>>>>>> --- > >>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 16 +++++++++ > >>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 +++++ > >>>>>>>>>> .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 35 ++++++++++++++++--- > >>>>>>>>>> drivers/gpu/drm/msm/dp/dp_display.c | 12 +++++++ > >>>>>>>>>> drivers/gpu/drm/msm/msm_drv.h | 9 ++++- > >>>>>>>>>> 5 files changed, 75 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 b0896814c1562..99ec53446ad21 100644 > >>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >>>>>>>>>> @@ -222,6 +222,22 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = { > >>>>>>>>>> 15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10 > >>>>>>>>>> }; > >>>>>>>>>> +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, > >>>>>>>>>> const struct drm_display_mode *mode) > >>>>>>>>>> +{ > >>>>>>>>>> + const struct dpu_encoder_virt *dpu_enc; > >>>>>>>>>> + const struct msm_display_info *disp_info; > >>>>>>>>>> + struct msm_drm_private *priv; > >>>>>>>>>> + > >>>>>>>>>> + dpu_enc = to_dpu_encoder_virt(drm_enc); > >>>>>>>>>> + disp_info = &dpu_enc->disp_info; > >>>>>>>>>> + priv = drm_enc->dev->dev_private; > >>>>>>>>>> + > >>>>>>>>>> + if (disp_info->intf_type == INTF_DP && > >>>>>>>>>> + msm_dp_is_yuv_420_enabled(priv->dp[disp_info->h_tile_instance[0]], > >>>>>>>>>> mode)) > >>>>>>>>> > >>>>>>>>> This should not require interacting with DP. If we got here, we must > >>>>>>>>> be sure that 4:2:0 is supported and can be configured. > >>>>>>>> Ack. Will drop this function and only check for if the mode is YUV420. > >>>>>>>>> > >>>>>>>>>> + return DRM_FORMAT_YUV420; > >>>>>>>>>> + > >>>>>>>>>> + return DRM_FORMAT_RGB888; > >>>>>>>>>> +} > >>>>>>>>>> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder > >>>>>>>>>> *drm_enc) > >>>>>>>>>> { > >>>>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >>>>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >>>>>>>>>> index 7b4afa71f1f96..62255d0aa4487 100644 > >>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > >>>>>>>>>> @@ -162,6 +162,14 @@ int dpu_encoder_get_vsync_count(struct > >>>>>>>>>> drm_encoder *drm_enc); > >>>>>>>>>> */ > >>>>>>>>>> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder > >>>>>>>>>> *drm_enc); > >>>>>>>>>> +/** > >>>>>>>>>> + * dpu_encoder_get_drm_fmt - return DRM fourcc format > >>>>>>>>>> + * @drm_enc: Pointer to previously created drm encoder structure > >>>>>>>>>> + * @mode: Corresponding drm_display_mode for dpu encoder > >>>>>>>>>> + */ > >>>>>>>>>> +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, > >>>>>>>>>> + const struct drm_display_mode *mode); > >>>>>>>>>> + > >>>>>>>>>> /** > >>>>>>>>>> * dpu_encoder_get_crc_values_cnt - get number of physical encoders > >>>>>>>>>> contained > >>>>>>>>>> * in virtual encoder that can collect CRC values > >>>>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > >>>>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > >>>>>>>>>> index e284bf448bdda..a1dde0ff35dc8 100644 > >>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > >>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > >>>>>>>>>> @@ -234,6 +234,7 @@ static void > >>>>>>>>>> dpu_encoder_phys_vid_setup_timing_engine( > >>>>>>>>>> { > >>>>>>>>>> struct drm_display_mode mode; > >>>>>>>>>> struct dpu_hw_intf_timing_params timing_params = { 0 }; > >>>>>>>>>> + struct dpu_hw_cdm *hw_cdm; > >>>>>>>>>> const struct dpu_format *fmt = NULL; > >>>>>>>>>> u32 fmt_fourcc = DRM_FORMAT_RGB888; > >>>>>>>>>> unsigned long lock_flags; > >>>>>>>>>> @@ -254,17 +255,26 @@ static void > >>>>>>>>>> dpu_encoder_phys_vid_setup_timing_engine( > >>>>>>>>>> DPU_DEBUG_VIDENC(phys_enc, "enabling mode:\n"); > >>>>>>>>>> drm_mode_debug_printmodeline(&mode); > >>>>>>>>>> - if (phys_enc->split_role != ENC_ROLE_SOLO) { > >>>>>>>>>> + hw_cdm = phys_enc->hw_cdm; > >>>>>>>>>> + if (hw_cdm) { > >>>>>>>>>> + intf_cfg.cdm = hw_cdm->idx; > >>>>>>>>>> + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, &mode); > >>>>>>>>>> + } > >>>>>>>>>> + > >>>>>>>>>> + if (phys_enc->split_role != ENC_ROLE_SOLO || > >>>>>>>>>> + dpu_encoder_get_drm_fmt(phys_enc->parent, &mode) == > >>>>>>>>>> DRM_FORMAT_YUV420) { > >>>>>>>>>> mode.hdisplay >>= 1; > >>>>>>>>>> mode.htotal >>= 1; > >>>>>>>>>> mode.hsync_start >>= 1; > >>>>>>>>>> mode.hsync_end >>= 1; > >>>>>>>>>> + mode.hskew >>= 1; > >>>>>>>>> > >>>>>>>>> Separate patch. > >>>>>>>> Ack. > >>>>>>>>> > >>>>>>>>>> DPU_DEBUG_VIDENC(phys_enc, > >>>>>>>>>> - "split_role %d, halve horizontal %d %d %d %d\n", > >>>>>>>>>> + "split_role %d, halve horizontal %d %d %d %d %d\n", > >>>>>>>>>> phys_enc->split_role, > >>>>>>>>>> mode.hdisplay, mode.htotal, > >>>>>>>>>> - mode.hsync_start, mode.hsync_end); > >>>>>>>>>> + mode.hsync_start, mode.hsync_end, > >>>>>>>>>> + mode.hskew); > >>>>>>>>>> } > >>>>>>>>>> drm_mode_to_intf_timing_params(phys_enc, &mode, &timing_params); > >>>>>>>>>> @@ -412,8 +422,15 @@ static int dpu_encoder_phys_vid_control_vblank_irq( > >>>>>>>>>> static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys > >>>>>>>>>> *phys_enc) > >>>>>>>>>> { > >>>>>>>>>> struct dpu_hw_ctl *ctl; > >>>>>>>>>> + struct dpu_hw_cdm *hw_cdm; > >>>>>>>>>> + const struct dpu_format *fmt = NULL; > >>>>>>>>>> + u32 fmt_fourcc = DRM_FORMAT_RGB888; > >>>>>>>>>> ctl = phys_enc->hw_ctl; > >>>>>>>>>> + hw_cdm = phys_enc->hw_cdm; > >>>>>>>>>> + if (hw_cdm) > >>>>>>>>>> + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, > >>>>>>>>>> &phys_enc->cached_mode); > >>>>>>>>>> + fmt = dpu_get_dpu_format(fmt_fourcc); > >>>>>>>>>> DPU_DEBUG_VIDENC(phys_enc, "\n"); > >>>>>>>>>> @@ -422,6 +439,8 @@ static void dpu_encoder_phys_vid_enable(struct > >>>>>>>>>> dpu_encoder_phys *phys_enc) > >>>>>>>>>> dpu_encoder_helper_split_config(phys_enc, > >>>>>>>>>> phys_enc->hw_intf->idx); > >>>>>>>>>> + dpu_encoder_helper_phys_setup_cdm(phys_enc, fmt, > >>>>>>>>>> CDM_CDWN_OUTPUT_HDMI); > >>>>>>>>> > >>>>>>>>> If there is no CDM, why do we need to call this? > >>>>>>>> Inside of dpu_encoder_helper_phys_setup_cdm(), there's a check to see if > >>>>>>>> there is a hw_cdm. If there is not, then it immediately exits the function. > >>>>>>>>> > >>>>>>>>>> + > >>>>>>>>>> dpu_encoder_phys_vid_setup_timing_engine(phys_enc); > >>>>>>>>>> /* > >>>>>>>>>> @@ -437,7 +456,15 @@ static void dpu_encoder_phys_vid_enable(struct > >>>>>>>>>> dpu_encoder_phys *phys_enc) > >>>>>>>>>> if (ctl->ops.update_pending_flush_merge_3d && > >>>>>>>>>> phys_enc->hw_pp->merge_3d) > >>>>>>>>>> ctl->ops.update_pending_flush_merge_3d(ctl, > >>>>>>>>>> phys_enc->hw_pp->merge_3d->idx); > >>>>>>>>>> - if (ctl->ops.update_pending_flush_periph && > >>>>>>>>>> phys_enc->hw_intf->cap->type == INTF_DP) > >>>>>>>>>> + if (ctl->ops.update_pending_flush_cdm && phys_enc->hw_cdm) > >>>>>>>>>> + ctl->ops.update_pending_flush_cdm(ctl, hw_cdm->idx); > >>>>>>>>>> + > >>>>>>>>>> + /* > >>>>>>>>>> + * Peripheral flush must be updated whenever flushing SDP > >>>>>>>>>> packets is needed. > >>>>>>>>>> + * SDP packets are required for any YUV format (YUV420, YUV422, > >>>>>>>>>> YUV444). > >>>>>>>>>> + */ > >>>>>>>>>> + if (ctl->ops.update_pending_flush_periph && > >>>>>>>>>> phys_enc->hw_intf->cap->type == INTF_DP && > >>>>>>>>>> + phys_enc->hw_cdm) > >>>>>>>>>> ctl->ops.update_pending_flush_periph(ctl, > >>>>>>>>>> phys_enc->hw_intf->idx); > >>>>>>>>> > >>>>>>>>> Should there be a flush if we are switching from YUV 420 to RGB mode? > >>>>>>>> We only need to flush for the sdp packet, but for msa we do not need to > >>>>>>>> flush. > >>>>>>> > >>>>>>> What about having SDP with RGB as colorimetry? In other words, if > >>>>>>> there is a decision point, this one looks incorrect. > >>>>>>> > >>>>>> > >>>>>> There are two ways to do it: > >>>>>> > >>>>>> 1) Use SDP for both RGB and YUV as that supports both. If we implement > >>>>>> this policy, then what you are asking for is correct that we will need > >>>>>> SDP even to switch back to RGB. But to implement this we will also need > >>>>>> to have some sort of state management in the encoder layer about what is > >>>>>> the current encoder fmt Vs what is the prev fmt and then trigger > >>>>>> peripheral flush only during transitions from RGB to YUV and vice-versa > >>>>>> > >>>>>> 2) Use SDP only for YUV because MSA does not support YUV formats and use > >>>>>> MSA for RGB > >>>>>> > >>>>>> We decided to implement (2) and there is no significant impact of > >>>>>> switching between MSA and SDPs but state management becomes easier. > >>>>> > >>>>> Yes. However as you wrote, there might be other usecases concerning > >>>>> SDP. Having this in mind, it sounds like the driver should decide > >>>>> whether to flush peripheral at a different place (when the SDP > >>>>> infoframe is being updated?). And the dpu_encoder_phys_vid_enable() > >>>>> should use this previous decision. Maybe this should be a part of > >>>>> msm_dp_ API, something like msm_dp_needs_peripheral_flush()? > >>>>> > >>>> > >>>> Correct. The decision to flush peripheral or not certainly comes from > >>>> the peripheral itself . In this case its DP. > >>>> > >>>> I think perhaps the usage of hw_cdm here makes it hard to understand > >>>> that but the idea behind this was that in the change "drm/msm/dpu: > >>>> reserve CDM blocks for DP if mode is YUV420 ", hw_cdm is assigned only > >>>> if msm_dp_is_yuv_420_enabled() returns true. > >>> > >>> Yes. > >>> > >>>> > >>>> So in some sense, the API you are asking for is already > >>>> msm_dp_is_yuv_420_enabled() but we can rename that to > >>>> msm_dp_needs_periph_flush(). > >>> > >>> No. This leaks details. We might need peripheral flush for other > >>> reasons. So this decision should be made inside the DP driver. > >>> BTW, is there a need to flush-peripheral each time the INTF gets > >>> enabled? When some bits of configuration were updated (like SDP > >>> infoframe or CDM config)? > >>> > >> > >> Not really leaking any details. Interface decides when DPU's flush bit > >> needs to be invoked. All the conditions needing the flush can be > >> absorbed within msm_dp_needs_periph_flush() which will be within DP > >> driver. So I dont see any issue with that. > > > > As long as we have two distinct functions: msm_dp_needs_periph_flush() > > and msm_dp_is_yuv420_enabled(), it's fine with me. > > > > I am okay with it being distinct but in this series and till DP DSC is > posted, we have only YUV420 SDP use-case for peripheral flush. > > So are you okay with the fact that the code will look something like > > bool msm_dp_needs_periph_flush() { > return msm_dp_is_yuv420_enabled(); > } Yes. The reason is that if at some point conditions for periph flush get changed, you won't have to change the API between DPU and DP drivers. > > >> > >> The peripheral flush needs to be invoked whenever SDP/PPS packets need > >> to be sent out at the very least. Those are the two main use-cases so far. > >> > >> Not necessarily each time the INTF is enabled. > >> > >> But .... > >> > >> For PPS, we would typically enable DSC from the first frame itself and > >> we dont support dynamically turning DSC ON/OFF so its a fair assumption > >> that we need to the peripheral flush for DSC only during enable. > >> > >> For SDP, so today we enforce a modeset when we switch from a case of > >> needing cdm / not needing cdm and vice-versa. So even this use-case > >> should be accompanied by a disable()/enable(). > > > > My point was that there might be other changes to the SDP infoframe. > > > >> > >> So are you suggesting that we unconditionally just do peripheral_flush > >> from enable()? > >> > >> I need to do some checking but maybe .... > > > > This might be an option too. I don't know what delays / issues it can cause. > > > > I will check this if the other option we discussed above does not work > for you. > > >> > >>>> > >>>> The issue here is the phys layer cannot call msm_dp_is_yuv_420_enabled() > >>>> so its kind of round-about by checking hw_cdm. > >>> > >>> Which maybe means that I should finish phys / encoder rework. > >>> > >> > >> Ugh no. Not another rework just for this. > > > > It is pending for the CTL flush story. > > > >> > >>>> > >>>> The check is not entirely wrong because for DP, we need hw_cdm for all > >>>> YUV formats and not just 420 but the relationship is maybe not clear but > >>>> I thought the comment above that would help a bit: > >>>> > >>>> /* > >>>> * Peripheral flush must be updated whenever flushing SDP packets is > >>>> needed. > >>>> * SDP packets are required for any YUV format (YUV420, YUV422, YUV444). > >>>> */ > >>>> > >>>> The only other way I can think of is maybe we need to introduce a new > >>>> phys variable called phys_enc->needs_update_periph_flush and we can set > >>>> that in dpu_encoder.c as that can call msm_dp_needs_periph_flush(). > >>>> > >>>> What do you think of this way? > >>> > >>> Let me understand first the requirements for peripheral flush (see the > >>> questions above).
On 1/31/2024 7:17 PM, Dmitry Baryshkov wrote: > On Thu, 1 Feb 2024 at 03:30, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 1/29/2024 3:44 PM, Dmitry Baryshkov wrote: >>> On Mon, 29 Jan 2024 at 09:08, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >>>> >>>> On 1/28/2024 10:12 PM, Dmitry Baryshkov wrote: >>>>> On Mon, 29 Jan 2024 at 07:03, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 1/28/2024 7:42 PM, Dmitry Baryshkov wrote: >>>>>>> On Mon, 29 Jan 2024 at 04:58, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On 1/27/2024 9:55 PM, Dmitry Baryshkov wrote: >>>>>>>>> On Sun, 28 Jan 2024 at 07:48, Paloma Arellano <quic_parellan@quicinc.com> wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On 1/25/2024 1:57 PM, Dmitry Baryshkov wrote: >>>>>>>>>>> On 25/01/2024 21:38, Paloma Arellano wrote: >>>>>>>>>>>> Adjust the encoder format programming in the case of video mode for DP >>>>>>>>>>>> to accommodate CDM related changes. >>>>>>>>>>>> >>>>>>>>>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> >>>>>>>>>>>> --- >>>>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 16 +++++++++ >>>>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 +++++ >>>>>>>>>>>> .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 35 ++++++++++++++++--- >>>>>>>>>>>> drivers/gpu/drm/msm/dp/dp_display.c | 12 +++++++ >>>>>>>>>>>> drivers/gpu/drm/msm/msm_drv.h | 9 ++++- >>>>>>>>>>>> 5 files changed, 75 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 b0896814c1562..99ec53446ad21 100644 >>>>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>>>>>>>>>>> @@ -222,6 +222,22 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = { >>>>>>>>>>>> 15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10 >>>>>>>>>>>> }; >>>>>>>>>>>> +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, >>>>>>>>>>>> const struct drm_display_mode *mode) >>>>>>>>>>>> +{ >>>>>>>>>>>> + const struct dpu_encoder_virt *dpu_enc; >>>>>>>>>>>> + const struct msm_display_info *disp_info; >>>>>>>>>>>> + struct msm_drm_private *priv; >>>>>>>>>>>> + >>>>>>>>>>>> + dpu_enc = to_dpu_encoder_virt(drm_enc); >>>>>>>>>>>> + disp_info = &dpu_enc->disp_info; >>>>>>>>>>>> + priv = drm_enc->dev->dev_private; >>>>>>>>>>>> + >>>>>>>>>>>> + if (disp_info->intf_type == INTF_DP && >>>>>>>>>>>> + msm_dp_is_yuv_420_enabled(priv->dp[disp_info->h_tile_instance[0]], >>>>>>>>>>>> mode)) >>>>>>>>>>> >>>>>>>>>>> This should not require interacting with DP. If we got here, we must >>>>>>>>>>> be sure that 4:2:0 is supported and can be configured. >>>>>>>>>> Ack. Will drop this function and only check for if the mode is YUV420. >>>>>>>>>>> >>>>>>>>>>>> + return DRM_FORMAT_YUV420; >>>>>>>>>>>> + >>>>>>>>>>>> + return DRM_FORMAT_RGB888; >>>>>>>>>>>> +} >>>>>>>>>>>> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder >>>>>>>>>>>> *drm_enc) >>>>>>>>>>>> { >>>>>>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>>>>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>>>>>>>>>> index 7b4afa71f1f96..62255d0aa4487 100644 >>>>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h >>>>>>>>>>>> @@ -162,6 +162,14 @@ int dpu_encoder_get_vsync_count(struct >>>>>>>>>>>> drm_encoder *drm_enc); >>>>>>>>>>>> */ >>>>>>>>>>>> bool dpu_encoder_is_widebus_enabled(const struct drm_encoder >>>>>>>>>>>> *drm_enc); >>>>>>>>>>>> +/** >>>>>>>>>>>> + * dpu_encoder_get_drm_fmt - return DRM fourcc format >>>>>>>>>>>> + * @drm_enc: Pointer to previously created drm encoder structure >>>>>>>>>>>> + * @mode: Corresponding drm_display_mode for dpu encoder >>>>>>>>>>>> + */ >>>>>>>>>>>> +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, >>>>>>>>>>>> + const struct drm_display_mode *mode); >>>>>>>>>>>> + >>>>>>>>>>>> /** >>>>>>>>>>>> * dpu_encoder_get_crc_values_cnt - get number of physical encoders >>>>>>>>>>>> contained >>>>>>>>>>>> * in virtual encoder that can collect CRC values >>>>>>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c >>>>>>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c >>>>>>>>>>>> index e284bf448bdda..a1dde0ff35dc8 100644 >>>>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c >>>>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c >>>>>>>>>>>> @@ -234,6 +234,7 @@ static void >>>>>>>>>>>> dpu_encoder_phys_vid_setup_timing_engine( >>>>>>>>>>>> { >>>>>>>>>>>> struct drm_display_mode mode; >>>>>>>>>>>> struct dpu_hw_intf_timing_params timing_params = { 0 }; >>>>>>>>>>>> + struct dpu_hw_cdm *hw_cdm; >>>>>>>>>>>> const struct dpu_format *fmt = NULL; >>>>>>>>>>>> u32 fmt_fourcc = DRM_FORMAT_RGB888; >>>>>>>>>>>> unsigned long lock_flags; >>>>>>>>>>>> @@ -254,17 +255,26 @@ static void >>>>>>>>>>>> dpu_encoder_phys_vid_setup_timing_engine( >>>>>>>>>>>> DPU_DEBUG_VIDENC(phys_enc, "enabling mode:\n"); >>>>>>>>>>>> drm_mode_debug_printmodeline(&mode); >>>>>>>>>>>> - if (phys_enc->split_role != ENC_ROLE_SOLO) { >>>>>>>>>>>> + hw_cdm = phys_enc->hw_cdm; >>>>>>>>>>>> + if (hw_cdm) { >>>>>>>>>>>> + intf_cfg.cdm = hw_cdm->idx; >>>>>>>>>>>> + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, &mode); >>>>>>>>>>>> + } >>>>>>>>>>>> + >>>>>>>>>>>> + if (phys_enc->split_role != ENC_ROLE_SOLO || >>>>>>>>>>>> + dpu_encoder_get_drm_fmt(phys_enc->parent, &mode) == >>>>>>>>>>>> DRM_FORMAT_YUV420) { >>>>>>>>>>>> mode.hdisplay >>= 1; >>>>>>>>>>>> mode.htotal >>= 1; >>>>>>>>>>>> mode.hsync_start >>= 1; >>>>>>>>>>>> mode.hsync_end >>= 1; >>>>>>>>>>>> + mode.hskew >>= 1; >>>>>>>>>>> >>>>>>>>>>> Separate patch. >>>>>>>>>> Ack. >>>>>>>>>>> >>>>>>>>>>>> DPU_DEBUG_VIDENC(phys_enc, >>>>>>>>>>>> - "split_role %d, halve horizontal %d %d %d %d\n", >>>>>>>>>>>> + "split_role %d, halve horizontal %d %d %d %d %d\n", >>>>>>>>>>>> phys_enc->split_role, >>>>>>>>>>>> mode.hdisplay, mode.htotal, >>>>>>>>>>>> - mode.hsync_start, mode.hsync_end); >>>>>>>>>>>> + mode.hsync_start, mode.hsync_end, >>>>>>>>>>>> + mode.hskew); >>>>>>>>>>>> } >>>>>>>>>>>> drm_mode_to_intf_timing_params(phys_enc, &mode, &timing_params); >>>>>>>>>>>> @@ -412,8 +422,15 @@ static int dpu_encoder_phys_vid_control_vblank_irq( >>>>>>>>>>>> static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys >>>>>>>>>>>> *phys_enc) >>>>>>>>>>>> { >>>>>>>>>>>> struct dpu_hw_ctl *ctl; >>>>>>>>>>>> + struct dpu_hw_cdm *hw_cdm; >>>>>>>>>>>> + const struct dpu_format *fmt = NULL; >>>>>>>>>>>> + u32 fmt_fourcc = DRM_FORMAT_RGB888; >>>>>>>>>>>> ctl = phys_enc->hw_ctl; >>>>>>>>>>>> + hw_cdm = phys_enc->hw_cdm; >>>>>>>>>>>> + if (hw_cdm) >>>>>>>>>>>> + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, >>>>>>>>>>>> &phys_enc->cached_mode); >>>>>>>>>>>> + fmt = dpu_get_dpu_format(fmt_fourcc); >>>>>>>>>>>> DPU_DEBUG_VIDENC(phys_enc, "\n"); >>>>>>>>>>>> @@ -422,6 +439,8 @@ static void dpu_encoder_phys_vid_enable(struct >>>>>>>>>>>> dpu_encoder_phys *phys_enc) >>>>>>>>>>>> dpu_encoder_helper_split_config(phys_enc, >>>>>>>>>>>> phys_enc->hw_intf->idx); >>>>>>>>>>>> + dpu_encoder_helper_phys_setup_cdm(phys_enc, fmt, >>>>>>>>>>>> CDM_CDWN_OUTPUT_HDMI); >>>>>>>>>>> >>>>>>>>>>> If there is no CDM, why do we need to call this? >>>>>>>>>> Inside of dpu_encoder_helper_phys_setup_cdm(), there's a check to see if >>>>>>>>>> there is a hw_cdm. If there is not, then it immediately exits the function. >>>>>>>>>>> >>>>>>>>>>>> + >>>>>>>>>>>> dpu_encoder_phys_vid_setup_timing_engine(phys_enc); >>>>>>>>>>>> /* >>>>>>>>>>>> @@ -437,7 +456,15 @@ static void dpu_encoder_phys_vid_enable(struct >>>>>>>>>>>> dpu_encoder_phys *phys_enc) >>>>>>>>>>>> if (ctl->ops.update_pending_flush_merge_3d && >>>>>>>>>>>> phys_enc->hw_pp->merge_3d) >>>>>>>>>>>> ctl->ops.update_pending_flush_merge_3d(ctl, >>>>>>>>>>>> phys_enc->hw_pp->merge_3d->idx); >>>>>>>>>>>> - if (ctl->ops.update_pending_flush_periph && >>>>>>>>>>>> phys_enc->hw_intf->cap->type == INTF_DP) >>>>>>>>>>>> + if (ctl->ops.update_pending_flush_cdm && phys_enc->hw_cdm) >>>>>>>>>>>> + ctl->ops.update_pending_flush_cdm(ctl, hw_cdm->idx); >>>>>>>>>>>> + >>>>>>>>>>>> + /* >>>>>>>>>>>> + * Peripheral flush must be updated whenever flushing SDP >>>>>>>>>>>> packets is needed. >>>>>>>>>>>> + * SDP packets are required for any YUV format (YUV420, YUV422, >>>>>>>>>>>> YUV444). >>>>>>>>>>>> + */ >>>>>>>>>>>> + if (ctl->ops.update_pending_flush_periph && >>>>>>>>>>>> phys_enc->hw_intf->cap->type == INTF_DP && >>>>>>>>>>>> + phys_enc->hw_cdm) >>>>>>>>>>>> ctl->ops.update_pending_flush_periph(ctl, >>>>>>>>>>>> phys_enc->hw_intf->idx); >>>>>>>>>>> >>>>>>>>>>> Should there be a flush if we are switching from YUV 420 to RGB mode? >>>>>>>>>> We only need to flush for the sdp packet, but for msa we do not need to >>>>>>>>>> flush. >>>>>>>>> >>>>>>>>> What about having SDP with RGB as colorimetry? In other words, if >>>>>>>>> there is a decision point, this one looks incorrect. >>>>>>>>> >>>>>>>> >>>>>>>> There are two ways to do it: >>>>>>>> >>>>>>>> 1) Use SDP for both RGB and YUV as that supports both. If we implement >>>>>>>> this policy, then what you are asking for is correct that we will need >>>>>>>> SDP even to switch back to RGB. But to implement this we will also need >>>>>>>> to have some sort of state management in the encoder layer about what is >>>>>>>> the current encoder fmt Vs what is the prev fmt and then trigger >>>>>>>> peripheral flush only during transitions from RGB to YUV and vice-versa >>>>>>>> >>>>>>>> 2) Use SDP only for YUV because MSA does not support YUV formats and use >>>>>>>> MSA for RGB >>>>>>>> >>>>>>>> We decided to implement (2) and there is no significant impact of >>>>>>>> switching between MSA and SDPs but state management becomes easier. >>>>>>> >>>>>>> Yes. However as you wrote, there might be other usecases concerning >>>>>>> SDP. Having this in mind, it sounds like the driver should decide >>>>>>> whether to flush peripheral at a different place (when the SDP >>>>>>> infoframe is being updated?). And the dpu_encoder_phys_vid_enable() >>>>>>> should use this previous decision. Maybe this should be a part of >>>>>>> msm_dp_ API, something like msm_dp_needs_peripheral_flush()? >>>>>>> >>>>>> >>>>>> Correct. The decision to flush peripheral or not certainly comes from >>>>>> the peripheral itself . In this case its DP. >>>>>> >>>>>> I think perhaps the usage of hw_cdm here makes it hard to understand >>>>>> that but the idea behind this was that in the change "drm/msm/dpu: >>>>>> reserve CDM blocks for DP if mode is YUV420 ", hw_cdm is assigned only >>>>>> if msm_dp_is_yuv_420_enabled() returns true. >>>>> >>>>> Yes. >>>>> >>>>>> >>>>>> So in some sense, the API you are asking for is already >>>>>> msm_dp_is_yuv_420_enabled() but we can rename that to >>>>>> msm_dp_needs_periph_flush(). >>>>> >>>>> No. This leaks details. We might need peripheral flush for other >>>>> reasons. So this decision should be made inside the DP driver. >>>>> BTW, is there a need to flush-peripheral each time the INTF gets >>>>> enabled? When some bits of configuration were updated (like SDP >>>>> infoframe or CDM config)? >>>>> >>>> >>>> Not really leaking any details. Interface decides when DPU's flush bit >>>> needs to be invoked. All the conditions needing the flush can be >>>> absorbed within msm_dp_needs_periph_flush() which will be within DP >>>> driver. So I dont see any issue with that. >>> >>> As long as we have two distinct functions: msm_dp_needs_periph_flush() >>> and msm_dp_is_yuv420_enabled(), it's fine with me. >>> >> >> I am okay with it being distinct but in this series and till DP DSC is >> posted, we have only YUV420 SDP use-case for peripheral flush. >> >> So are you okay with the fact that the code will look something like >> >> bool msm_dp_needs_periph_flush() { >> return msm_dp_is_yuv420_enabled(); >> } > > Yes. The reason is that if at some point conditions for periph flush > get changed, you won't have to change the API between DPU and DP > drivers. > Ack, I am okay with this. >> >>>> >>>> The peripheral flush needs to be invoked whenever SDP/PPS packets need >>>> to be sent out at the very least. Those are the two main use-cases so far. >>>> >>>> Not necessarily each time the INTF is enabled. >>>> >>>> But .... >>>> >>>> For PPS, we would typically enable DSC from the first frame itself and >>>> we dont support dynamically turning DSC ON/OFF so its a fair assumption >>>> that we need to the peripheral flush for DSC only during enable. >>>> >>>> For SDP, so today we enforce a modeset when we switch from a case of >>>> needing cdm / not needing cdm and vice-versa. So even this use-case >>>> should be accompanied by a disable()/enable(). >>> >>> My point was that there might be other changes to the SDP infoframe. >>> >>>> >>>> So are you suggesting that we unconditionally just do peripheral_flush >>>> from enable()? >>>> >>>> I need to do some checking but maybe .... >>> >>> This might be an option too. I don't know what delays / issues it can cause. >>> >> >> I will check this if the other option we discussed above does not work >> for you. >> >>>> >>>>>> >>>>>> The issue here is the phys layer cannot call msm_dp_is_yuv_420_enabled() >>>>>> so its kind of round-about by checking hw_cdm. >>>>> >>>>> Which maybe means that I should finish phys / encoder rework. >>>>> >>>> >>>> Ugh no. Not another rework just for this. >>> >>> It is pending for the CTL flush story. >>> >>>> >>>>>> >>>>>> The check is not entirely wrong because for DP, we need hw_cdm for all >>>>>> YUV formats and not just 420 but the relationship is maybe not clear but >>>>>> I thought the comment above that would help a bit: >>>>>> >>>>>> /* >>>>>> * Peripheral flush must be updated whenever flushing SDP packets is >>>>>> needed. >>>>>> * SDP packets are required for any YUV format (YUV420, YUV422, YUV444). >>>>>> */ >>>>>> >>>>>> The only other way I can think of is maybe we need to introduce a new >>>>>> phys variable called phys_enc->needs_update_periph_flush and we can set >>>>>> that in dpu_encoder.c as that can call msm_dp_needs_periph_flush(). >>>>>> >>>>>> What do you think of this way? >>>>> >>>>> Let me understand first the requirements for peripheral flush (see the >>>>> questions above). > > >
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index b0896814c1562..99ec53446ad21 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -222,6 +222,22 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = { 15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10 }; +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, const struct drm_display_mode *mode) +{ + const struct dpu_encoder_virt *dpu_enc; + const struct msm_display_info *disp_info; + struct msm_drm_private *priv; + + dpu_enc = to_dpu_encoder_virt(drm_enc); + disp_info = &dpu_enc->disp_info; + priv = drm_enc->dev->dev_private; + + if (disp_info->intf_type == INTF_DP && + msm_dp_is_yuv_420_enabled(priv->dp[disp_info->h_tile_instance[0]], mode)) + return DRM_FORMAT_YUV420; + + return DRM_FORMAT_RGB888; +} bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc) { diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h index 7b4afa71f1f96..62255d0aa4487 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h @@ -162,6 +162,14 @@ int dpu_encoder_get_vsync_count(struct drm_encoder *drm_enc); */ bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc); +/** + * dpu_encoder_get_drm_fmt - return DRM fourcc format + * @drm_enc: Pointer to previously created drm encoder structure + * @mode: Corresponding drm_display_mode for dpu encoder + */ +u32 dpu_encoder_get_drm_fmt(const struct drm_encoder *drm_enc, + const struct drm_display_mode *mode); + /** * dpu_encoder_get_crc_values_cnt - get number of physical encoders contained * in virtual encoder that can collect CRC values diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c index e284bf448bdda..a1dde0ff35dc8 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c @@ -234,6 +234,7 @@ static void dpu_encoder_phys_vid_setup_timing_engine( { struct drm_display_mode mode; struct dpu_hw_intf_timing_params timing_params = { 0 }; + struct dpu_hw_cdm *hw_cdm; const struct dpu_format *fmt = NULL; u32 fmt_fourcc = DRM_FORMAT_RGB888; unsigned long lock_flags; @@ -254,17 +255,26 @@ static void dpu_encoder_phys_vid_setup_timing_engine( DPU_DEBUG_VIDENC(phys_enc, "enabling mode:\n"); drm_mode_debug_printmodeline(&mode); - if (phys_enc->split_role != ENC_ROLE_SOLO) { + hw_cdm = phys_enc->hw_cdm; + if (hw_cdm) { + intf_cfg.cdm = hw_cdm->idx; + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, &mode); + } + + if (phys_enc->split_role != ENC_ROLE_SOLO || + dpu_encoder_get_drm_fmt(phys_enc->parent, &mode) == DRM_FORMAT_YUV420) { mode.hdisplay >>= 1; mode.htotal >>= 1; mode.hsync_start >>= 1; mode.hsync_end >>= 1; + mode.hskew >>= 1; DPU_DEBUG_VIDENC(phys_enc, - "split_role %d, halve horizontal %d %d %d %d\n", + "split_role %d, halve horizontal %d %d %d %d %d\n", phys_enc->split_role, mode.hdisplay, mode.htotal, - mode.hsync_start, mode.hsync_end); + mode.hsync_start, mode.hsync_end, + mode.hskew); } drm_mode_to_intf_timing_params(phys_enc, &mode, &timing_params); @@ -412,8 +422,15 @@ static int dpu_encoder_phys_vid_control_vblank_irq( static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys *phys_enc) { struct dpu_hw_ctl *ctl; + struct dpu_hw_cdm *hw_cdm; + const struct dpu_format *fmt = NULL; + u32 fmt_fourcc = DRM_FORMAT_RGB888; ctl = phys_enc->hw_ctl; + hw_cdm = phys_enc->hw_cdm; + if (hw_cdm) + fmt_fourcc = dpu_encoder_get_drm_fmt(phys_enc->parent, &phys_enc->cached_mode); + fmt = dpu_get_dpu_format(fmt_fourcc); DPU_DEBUG_VIDENC(phys_enc, "\n"); @@ -422,6 +439,8 @@ static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys *phys_enc) dpu_encoder_helper_split_config(phys_enc, phys_enc->hw_intf->idx); + dpu_encoder_helper_phys_setup_cdm(phys_enc, fmt, CDM_CDWN_OUTPUT_HDMI); + dpu_encoder_phys_vid_setup_timing_engine(phys_enc); /* @@ -437,7 +456,15 @@ static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys *phys_enc) if (ctl->ops.update_pending_flush_merge_3d && phys_enc->hw_pp->merge_3d) ctl->ops.update_pending_flush_merge_3d(ctl, phys_enc->hw_pp->merge_3d->idx); - if (ctl->ops.update_pending_flush_periph && phys_enc->hw_intf->cap->type == INTF_DP) + if (ctl->ops.update_pending_flush_cdm && phys_enc->hw_cdm) + ctl->ops.update_pending_flush_cdm(ctl, hw_cdm->idx); + + /* + * Peripheral flush must be updated whenever flushing SDP packets is needed. + * SDP packets are required for any YUV format (YUV420, YUV422, YUV444). + */ + if (ctl->ops.update_pending_flush_periph && phys_enc->hw_intf->cap->type == INTF_DP && + phys_enc->hw_cdm) ctl->ops.update_pending_flush_periph(ctl, phys_enc->hw_intf->idx); skip_flush: diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 6d764f5b08727..4329435518351 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -1399,6 +1399,18 @@ void __exit msm_dp_unregister(void) platform_driver_unregister(&dp_display_driver); } +bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display, + const struct drm_display_mode *mode) +{ + struct dp_display_private *dp; + const struct drm_display_info *info; + + dp = container_of(dp_display, struct dp_display_private, dp_display); + info = &dp_display->connector->display_info; + + return dp_panel_vsc_sdp_supported(dp->panel) && drm_mode_is_420_only(info, mode); +} + bool msm_dp_wide_bus_available(const struct msm_dp *dp_display) { struct dp_display_private *dp; diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h index 16a7cbc0b7dd8..b9581bd934e9e 100644 --- a/drivers/gpu/drm/msm/msm_drv.h +++ b/drivers/gpu/drm/msm/msm_drv.h @@ -387,7 +387,8 @@ void __exit msm_dp_unregister(void); int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, struct drm_encoder *encoder); void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_display); - +bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display, + const struct drm_display_mode *mode); bool msm_dp_wide_bus_available(const struct msm_dp *dp_display); #else @@ -409,6 +410,12 @@ static inline void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm { } +static inline bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display, + const struct drm_display_mode *mode) +{ + return false; +} + static inline bool msm_dp_wide_bus_available(const struct msm_dp *dp_display) { return false;
Adjust the encoder format programming in the case of video mode for DP to accommodate CDM related changes. Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 16 +++++++++ drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 +++++ .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 35 ++++++++++++++++--- drivers/gpu/drm/msm/dp/dp_display.c | 12 +++++++ drivers/gpu/drm/msm/msm_drv.h | 9 ++++- 5 files changed, 75 insertions(+), 5 deletions(-)